From c3ca6286aea550878f57610936800ca2a3ec6785 Mon Sep 17 00:00:00 2001 From: sec-check Date: Tue, 1 Sep 2026 02:36:46 -0400 Subject: [PATCH 001/190] [sec-check] fix: bind ttyd-hive.service to loopback by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The legacy host unit launched ttyd writable (-W) with --url-arg (-a), no credential, and no bind address — an unauthenticated shell as the service user on 0.0.0.0:7681 for any network peer. Bind 127.0.0.1 by default, mirroring the containerized default in src/deploy/entrypoint.sh (TTYD_BIND=127.0.0.1, hardened in #2178), and document how to expose it safely via drop-in override. Fixes #5434 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sec-check --- systemd/ttyd-hive.service | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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 From af2abc35a175e3d72077c1591089e26f2417d48b Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Tue, 1 Sep 2026 09:12:34 -0400 Subject: [PATCH 002/190] docs: add guide gap-verification rule; fix stale skill-delivery docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related documentation changes. 1. Guide policy: a documented limitation is not a documentation gap. The guide lane has been filing doc-gap issues at roughly 40% signal; every false positive failed the same way — reporting a gap whose answer was already in the docs, sometimes one cross-reference away. Adds a mandatory pre-filing check to the four guide policies that can record a finding (guide-issues, guide-full, guide-holdgated, guide-advisory) in both policy trees. guide.md is excluded: it never files. Extends the existing guide policy test to enforce the new guardrail across both trees, matching the precedent set by TestGuidePoliciesRequireCommandVerification. 2. Skill delivery is shipped; the docs said otherwise. src/docs/README.md still claimed the registry was loaded but not delivered and that populating it changed no agent behaviour. Delivery landed via #5313, #5323/#5227 and #5387. Documents the actual contract: opt-in per agent via skills:, registry-over-repo resolution, rendered into , 8 KiB whole-skill cap with over-cap skills dropped whole rather than truncated, both sources reloaded every kick. Also clears residual stale framing in skills.md and a stale citation in general-technical-review.md. 3. Adds the missing operator pointer for the HEADLESS_BACKENDS allowlist. The exclusion of agy, opencode and kilo is correct and documented, but neither doc told the operator what to do about it. Links the tracking issue and states the workaround. Fixes #5404 Fixes #5406 Signed-off-by: Andrew Anderson --- docs/backend-setup.md | 17 +++++++ src/docs/README.md | 2 +- src/docs/contributor-relay.md | 11 ++++ src/docs/general-technical-review.md | 2 +- src/docs/skills.md | 13 +++-- src/pkg/policies/defaults/guide-advisory.md | 30 +++++++++++ src/pkg/policies/defaults/guide-full.md | 30 +++++++++++ src/pkg/policies/defaults/guide-holdgated.md | 30 +++++++++++ src/pkg/policies/defaults/guide-issues.md | 30 +++++++++++ .../guide_command_verification_test.go | 50 +++++++++++++++++++ src/policies/guide-advisory.md | 30 +++++++++++ src/policies/guide-full.md | 30 +++++++++++ src/policies/guide-holdgated.md | 30 +++++++++++ src/policies/guide-issues.md | 30 +++++++++++ 14 files changed, 328 insertions(+), 7 deletions(-) diff --git a/docs/backend-setup.md b/docs/backend-setup.md index 3b2e0c57b..921332582 100644 --- a/docs/backend-setup.md +++ b/docs/backend-setup.md @@ -19,6 +19,23 @@ Hive validates backend names in `src/pkg/config` and launches CLIs in `src/pkg/a | `opencode` | `opencode` | Install the opencode CLI ([opencode.ai/docs](https://opencode.ai/docs/)) and run `opencode auth login`; the credential is written to `~/.local/share/opencode/auth.json`. Provider-agnostic (75+ providers) — the model provider is configured in opencode's own config/auth, not in Hive, so `AGENT_MODEL` is passed through as `provider/model` on the relay path (e.g. `export AGENT_MODEL=anthropic/claude-sonnet-4-6`). | **Contributor relay only; headless mode only.** Dispatches through `opencode run "" --auto` under `CONTRIBUTOR_MODE=headless`; there is no interactive-tmux wiring for opencode, so `CONTRIBUTOR_MODE=interactive` does not apply to it. `backend_perm_flag` maps opencode to `--auto`, opencode's unattended auto-approve flag. **Confinement note:** opencode has no OS-enforced filesystem sandbox of its own. `just contribute-hive opencode local` narrows it with a host-state command deny-list only (via opencode's own `permission.bash` config, the same command family the claude deny-list covers) — a floor, not a sandbox boundary. Container mode is the default and the stronger boundary. See [sandbox-isolation.md](../src/docs/sandbox-isolation.md#per-backend-confinement-on-the-contributor-local-path) for the full per-backend matrix. Set `HIVE_OPENCODE_DANGEROUSLY_ALLOW_HOST_STATE=1` to drop the deny-list. Not yet in `just contribute-k8s`'s headless-pod allowlist: whether the auth credential supports unattended use in a fresh pod is unverified, so it currently runs headless only on a host that has already signed in (same posture as `agy`). | | `kilo` | `kilo` | Install `@kilocode/cli` (pinned via `KILO_CLI_VERSION` in `src/Dockerfile.contributor`, currently `7.5.6`) and set credentials as environment values only — `KILO_AUTH_CONTENT` or `KILO_CONFIG_CONTENT`, or `KILO_API_KEY` (optional `KILO_ORG_ID`). No Kilo config directory is mounted; the Justfile's `PROVIDER_ENV_ARGS` mechanism forwards these four variables to the container by name, so the values themselves never appear in the container runtime's argv. | **Contributor relay only; headless mode only** (`CONTRIBUTOR_MODE=headless`; no interactive-tmux wiring). Dispatches through `kilo run "" --auto` (optional `--model provider/model`). `backend_perm_flag` maps kilo to `--auto`, kilo's unattended auto-approve flag. **Confinement note:** kilo has **no OS-enforced sandbox and no command deny-list floor** in `config/backends.conf` — `--auto` approves prompts, it is not a boundary. Local mode therefore **refuses to launch** kilo without `HIVE_KILO_DANGEROUSLY_RUN_UNCONFINED=1`, the same #4918 refusal gate as goose/agy/bob/pi/aider (unlike `opencode`, no host-state denylist exists for it — whether kilo honors an `OPENCODE_PERMISSION`-style config is unverified). Treat it as fully unconfined, same posture as goose/bob/pi/aider (see [sandbox-isolation.md](../src/docs/sandbox-isolation.md)). Kilo is intentionally **excluded from `just contribute-k8s`'s headless-pod allowlist** (`HEADLESS_BACKENDS="claude litellm copilot codex goose"`), pending independent credential and confinement verification. | +### Backends excluded from the headless K8s allowlist + +`just contribute-k8s` runs backends in a TTY-less pod and only permits the +backends in its `HEADLESS_BACKENDS` allowlist, currently +`claude litellm copilot codex goose` (`Justfile:1692`). `agy`, `opencode`, and +`kilo` are deliberately excluded: their credentials are not verified for +unattended use in a fresh pod, and `agy` in particular has no API-key mode at +all. + +If you need one of the excluded backends, either choose a supported headless +backend, or run it attended on the container or local path +(`just contribute-hive `), where an operator can complete an +interactive sign-in once. Tracking issue: +[#5406](https://github.com/kubestellar/hive/issues/5406). Whether these backends +can run headless at all remains an open question, so the allowlist is a +deliberate gate rather than an oversight. + ## IBM Bob headless setup `backend: bob` launches IBM bobshell (`bob`), the IBM watsonx Code Assistant CLI. In a Hive pod or contributor container it must use API-key auth: the default IBMid/W3ID browser SSO flow opens a browser and waits on a localhost callback, which a headless pod cannot satisfy, then times out after about three minutes. Hive checks for a key before launch and parks the agent with an actionable error instead of burning that timeout. diff --git a/src/docs/README.md b/src/docs/README.md index 184fc14f7..f552b45ba 100644 --- a/src/docs/README.md +++ b/src/docs/README.md @@ -67,7 +67,7 @@ Start with [Architecture](architecture.md) for the system overview, then use the - [Custom dashboard stylesheets](https://github.com/kubestellar/hive/blob/v4/src/docs/custom-stylesheets.md) — operator-supplied CSS for the dashboard and public snapshot. - [Portable AgentDefinition format](https://github.com/kubestellar/hive/blob/v4/src/AGENT-DEFINITION.md) — standalone YAML schema for importing/exporting agent definitions. - [Knowledge curator](https://github.com/kubestellar/hive/blob/v4/src/docs/knowledge-curator.md) — automatic fact extraction and promotion knobs, plus `knowledge.git_sources`: indexing a remote repo, layer semantics, private-repo auth (unsupported), and diagnosing a failed source. -- [Skill registry](skills.md) — the `/data/skills/` file format and front-matter fields. **Loaded and counted on the dashboard, but not yet delivered to agents**: populating it changes no agent's behaviour today. Use the knowledge curator for knowledge that actually reaches agents. +- [Skill registry](skills.md) — the `/data/skills/` file format and front-matter fields. **Delivered to agents today**: skills are opt-in per agent via `skills: [name, ...]`, resolved registry-first with an `AGENTS.md` repo-local fallback, and the rendered block is prepended to the kick's `${KNOWLEDGE}`. Both sources reload on every kick. An 8 KiB whole-skill cap applies; a skill that would exceed it is dropped whole, never truncated. - [AGENTS.md repo instructions](agents-md.md) — the per-repo `AGENTS.md` file format Hive's parser (`pkg/agentsmd`) understands, including front-matter `skills:` and inline `## Skill:` sections. **Wired into kicks, but needs a checkout**: Hive agents keep no clones, so set `project.checkouts_dir` to a directory holding one checkout per repo. Without it there is no root to read and injection stays a no-op, which is the default. - [Agent peer-awareness logging (pluk)](https://github.com/kubestellar/hive/blob/v4/src/docs/agent-logging.md) — pluk log format, `hive-panes`, availability, and retention. - [Strategy Lab (Nous)](https://github.com/kubestellar/hive/blob/v4/src/docs/strategy-lab.md) — experiment lifecycle, dashboard/API configuration, fast-fail bounds, and the gate-decision flow. No `nous:` block in `hive.yaml`. diff --git a/src/docs/contributor-relay.md b/src/docs/contributor-relay.md index 078fb67e2..6180d14cd 100644 --- a/src/docs/contributor-relay.md +++ b/src/docs/contributor-relay.md @@ -350,6 +350,17 @@ kubectl -n my-namespace rollout status deploy/hive-contributor The generated pod sets `CONTRIBUTOR_MODE=headless` because Kubernetes pods have no TTY; interactive tmux mode would stall. Headless mode is currently verified for `claude`, `litellm`, `copilot`, `codex`, `goose`, and `agy` (`agy -p`, verified on 1.1.13) — but **`agy` stays out of `just contribute-k8s`'s `HEADLESS_BACKENDS` allowlist regardless**: it signs in through an interactive Google OAuth flow with no API-key mode, and a pod has no way to complete that sign-in even once (unlike the container path, where an operator can attach and run `agy` interactively, or the relay can stage an already-signed-in `~/.gemini`). Headless `agy` is verified only on a host that has already signed in. `opencode` has a verified one-shot invocation (`opencode run ""`, [#4970](https://github.com/kubestellar/hive/issues/4970)) but is **not yet** in `just contribute-k8s`'s `HEADLESS_BACKENDS` allowlist: whether `opencode auth login`'s credential file supports non-interactive, unattended use in a fresh pod is unverified, so it currently runs headless on a host that has already signed in, the same posture as `agy`. The Deployment has one replica per registered contributor identity and uses readiness/liveness probes that read the relay's headless status file (`waiting`, `working`, `done` pass; missing/failed state fails). +**If you need `agy`, `opencode`, or `kilo` on the K8s path**, the allowlist is +`HEADLESS_BACKENDS="claude litellm copilot codex goose"` (`Justfile:1692`) and +`just contribute-k8s` refuses anything outside it. Two workarounds: pick a +supported headless backend, or run the backend attended on the container/local +path (`just contribute-hive `), where an operator can complete an +interactive sign-in. Tracking issue: +[#5406](https://github.com/kubestellar/hive/issues/5406). Whether these backends +can run unattended at all is still an open question — some may require an +interactive login that a pod cannot satisfy — so treat the allowlist as a +deliberate gate, not an oversight. + The generated Secret contains the registration token and `GH_TOKEN` as Kubernetes Secret data. Treat it as sensitive cluster-readable material and prefer a pinned image tag/digest for repeatable operation. ## How the hub picks work for contributors diff --git a/src/docs/general-technical-review.md b/src/docs/general-technical-review.md index df70a8a80..d29b2efac 100644 --- a/src/docs/general-technical-review.md +++ b/src/docs/general-technical-review.md @@ -307,7 +307,7 @@ Through `CHANGELOG.md`'s `## Unreleased` section, which explicitly asks for entr #### Explain how the project permits utilization of alpha and beta capabilities as part of a rollout. -No formal alpha/beta feature-flag maturity system (like Kubernetes feature gates) exists. The closest analogs, both explicit and self-labeled: (1) **release channels** — `edge`/`candidate`/`stable`, all currently synced to the same `v4-latest` digest but designed as the promotion mechanism for future channel divergence (`src/docs/release-channels.md`); (2) **doc-level status labels** on features that are design-only, partly shipped, or shipped-but-unwired — e.g. the skill registry is explicitly documented as "loaded and counted on the dashboard, but not yet delivered to agents" (`src/docs/README.md` skills.md entry), `AGENTS.md` parsing was labeled "parsed and tested, but not wired into kicks" until its checkout root was threaded in [#5227](https://github.com/kubestellar/hive/issues/5227), and the `design/` directory indexes longer-form records each carrying a status of shipped/partly-shipped/design-only/historical specifically so a proposal is never mistaken for current behavior (`src/docs/README.md` "Historical/design notes"). The agent self-healing watchdog is a concrete example of graduated rollout via an explicit mode ladder: it ships in `observe` mode (classifies and would-have-acted, but takes no action) and only promotes to `heal` (acts) on operator decision, with `HIVE_WATCHDOG_PAUSE=true` as a fleet-wide downgrade switch (`src/docs/agent-watchdog.md`). +No formal alpha/beta feature-flag maturity system (like Kubernetes feature gates) exists. The closest analogs, both explicit and self-labeled: (1) **release channels** — `edge`/`candidate`/`stable`, all currently synced to the same `v4-latest` digest but designed as the promotion mechanism for future channel divergence (`src/docs/release-channels.md`); (2) **doc-level status labels** on features that are design-only, partly shipped, or shipped-but-unwired — e.g. the skill registry carried "loaded and counted on the dashboard, but not yet delivered to agents" until delivery shipped and the label was retired (`src/docs/README.md` skills.md entry), `AGENTS.md` parsing was labeled "parsed and tested, but not wired into kicks" until its checkout root was threaded in [#5227](https://github.com/kubestellar/hive/issues/5227), and the `design/` directory indexes longer-form records each carrying a status of shipped/partly-shipped/design-only/historical specifically so a proposal is never mistaken for current behavior (`src/docs/README.md` "Historical/design notes"). The agent self-healing watchdog is a concrete example of graduated rollout via an explicit mode ladder: it ships in `observe` mode (classifies and would-have-acted, but takes no action) and only promotes to `heal` (acts) on operator decision, with `HIVE_WATCHDOG_PAUSE=true` as a fleet-wide downgrade switch (`src/docs/agent-watchdog.md`). ## Day 2 - Day-to-Day Operations Phase diff --git a/src/docs/skills.md b/src/docs/skills.md index 203ff3b05..3481b2c6c 100644 --- a/src/docs/skills.md +++ b/src/docs/skills.md @@ -5,9 +5,11 @@ review patterns, "how we do X here" — that an agent loads by name instead of having them pasted into every prompt. See [ADR-0012](adr/0012-skill-registry.md) for why it exists. -Skills reach agents **only when an agent declares them**. Dropping files in -`/data/skills/` makes them *available*; it does not change any agent's -behaviour until that agent's config names them (see +Skills **are delivered to agents today** — the scheduler resolves and injects +them on every kick (`primeSkills`). Delivery is opt-in: skills reach an agent +**only when that agent declares them**. Dropping files in `/data/skills/` makes +them *available*; it does not change any agent's behaviour until that agent's +config names them (see [Declaring skills on an agent](#declaring-skills-on-an-agent)). An agent with no `skills:` list is unaffected no matter what the directory contains. @@ -157,5 +159,6 @@ sources. - [ADR-0012: skill registry](adr/0012-skill-registry.md) — the architecture decision. ADRs are decision records, not operator guides. -- [Knowledge curator](knowledge-curator.md) — the mechanism that **does** - deliver knowledge to agents today. +- [Knowledge curator](knowledge-curator.md) — the complementary mechanism for + *factual* per-issue knowledge. Both it and the skill registry deliver into the + same `${KNOWLEDGE}` block of the kick. diff --git a/src/pkg/policies/defaults/guide-advisory.md b/src/pkg/policies/defaults/guide-advisory.md index 73dc49d8e..af3c8474a 100644 --- a/src/pkg/policies/defaults/guide-advisory.md +++ b/src/pkg/policies/defaults/guide-advisory.md @@ -56,6 +56,36 @@ bd update --set-metadata file="README.md" - `api` — undocumented public interfaces, config options, or environment variables - `contributing` — missing or incomplete contributor workflow docs +## Before Filing a Finding (MANDATORY) + +**A documented limitation is not a documentation gap.** Before you file anything, +grep the repo for the thing you claim is missing — including every doc the page +cross-links to. If the text is already there and it is accurate, your finding is +at most "this could be more prominent." That is polish, not a defect, and it does +not get an issue. + +The test is simple: **would a reader who actually hit this situation find the +answer?** If yes, the docs are working. Which file the answer lives in, whether +it is phrased the way you would phrase it, and whether a tracking issue exists +are matters of style and process — not documentation defects. + +Two corollaries: + +- **Search closed issues and the code before claiming nothing tracks this.** A + gap that was fixed yesterday is not a gap. `gh issue list --state all` and read + the current source, not just the doc. +- **Follow cross-references before concluding something is undocumented.** A page + that links onward to a deeper treatment has documented the thing. + +Recent calibration — findings that should never have been filed: an issue against +a doc that said a value is "not currently persisted across a process restart" +(that sentence *is* the documentation); an issue against "No on-call rotation," +an honest statement of current state; an issue about a topic the page cross-linked +to `security-model.md`, which covered it in more depth than the finding asked for; +and an issue claiming no tracking issue existed when one had closed hours earlier. +Findings that were correct all shared one trait: the missing thing had **literally +zero mentions** anywhere in the repo. Verify that before you file. + ## Workflow 1. Read the kick message for any specific documentation tasks diff --git a/src/pkg/policies/defaults/guide-full.md b/src/pkg/policies/defaults/guide-full.md index 5a362c11e..0bfec6602 100644 --- a/src/pkg/policies/defaults/guide-full.md +++ b/src/pkg/policies/defaults/guide-full.md @@ -57,6 +57,36 @@ bd create --title "" \ --type advisory --priority <0-3> --actor guide --external-ref "" ``` +## Before Filing a Finding (MANDATORY) + +**A documented limitation is not a documentation gap.** Before you file anything, +grep the repo for the thing you claim is missing — including every doc the page +cross-links to. If the text is already there and it is accurate, your finding is +at most "this could be more prominent." That is polish, not a defect, and it does +not get an issue. + +The test is simple: **would a reader who actually hit this situation find the +answer?** If yes, the docs are working. Which file the answer lives in, whether +it is phrased the way you would phrase it, and whether a tracking issue exists +are matters of style and process — not documentation defects. + +Two corollaries: + +- **Search closed issues and the code before claiming nothing tracks this.** A + gap that was fixed yesterday is not a gap. `gh issue list --state all` and read + the current source, not just the doc. +- **Follow cross-references before concluding something is undocumented.** A page + that links onward to a deeper treatment has documented the thing. + +Recent calibration — findings that should never have been filed: an issue against +a doc that said a value is "not currently persisted across a process restart" +(that sentence *is* the documentation); an issue against "No on-call rotation," +an honest statement of current state; an issue about a topic the page cross-linked +to `security-model.md`, which covered it in more depth than the finding asked for; +and an issue claiming no tracking issue existed when one had closed hours earlier. +Findings that were correct all shared one trait: the missing thing had **literally +zero mentions** anywhere in the repo. Verify that before you file. + ## Workflow 1. Read the kick message diff --git a/src/pkg/policies/defaults/guide-holdgated.md b/src/pkg/policies/defaults/guide-holdgated.md index c442b8356..388ddd9a2 100644 --- a/src/pkg/policies/defaults/guide-holdgated.md +++ b/src/pkg/policies/defaults/guide-holdgated.md @@ -58,6 +58,36 @@ bd create --title "" \ --type advisory --priority <0-3> --actor guide --external-ref "" ``` +## Before Filing a Finding (MANDATORY) + +**A documented limitation is not a documentation gap.** Before you file anything, +grep the repo for the thing you claim is missing — including every doc the page +cross-links to. If the text is already there and it is accurate, your finding is +at most "this could be more prominent." That is polish, not a defect, and it does +not get an issue. + +The test is simple: **would a reader who actually hit this situation find the +answer?** If yes, the docs are working. Which file the answer lives in, whether +it is phrased the way you would phrase it, and whether a tracking issue exists +are matters of style and process — not documentation defects. + +Two corollaries: + +- **Search closed issues and the code before claiming nothing tracks this.** A + gap that was fixed yesterday is not a gap. `gh issue list --state all` and read + the current source, not just the doc. +- **Follow cross-references before concluding something is undocumented.** A page + that links onward to a deeper treatment has documented the thing. + +Recent calibration — findings that should never have been filed: an issue against +a doc that said a value is "not currently persisted across a process restart" +(that sentence *is* the documentation); an issue against "No on-call rotation," +an honest statement of current state; an issue about a topic the page cross-linked +to `security-model.md`, which covered it in more depth than the finding asked for; +and an issue claiming no tracking issue existed when one had closed hours earlier. +Findings that were correct all shared one trait: the missing thing had **literally +zero mentions** anywhere in the repo. Verify that before you file. + ## Workflow 1. Read the kick message diff --git a/src/pkg/policies/defaults/guide-issues.md b/src/pkg/policies/defaults/guide-issues.md index c6425d026..51a7787f5 100644 --- a/src/pkg/policies/defaults/guide-issues.md +++ b/src/pkg/policies/defaults/guide-issues.md @@ -61,6 +61,36 @@ bd create --title "" \ Priority: 0 (no README/build instructions), 1 (missing setup/stale arch docs), 2 (missing contributor guide), 3 (typos/style) +## Before Filing a Finding (MANDATORY) + +**A documented limitation is not a documentation gap.** Before you file anything, +grep the repo for the thing you claim is missing — including every doc the page +cross-links to. If the text is already there and it is accurate, your finding is +at most "this could be more prominent." That is polish, not a defect, and it does +not get an issue. + +The test is simple: **would a reader who actually hit this situation find the +answer?** If yes, the docs are working. Which file the answer lives in, whether +it is phrased the way you would phrase it, and whether a tracking issue exists +are matters of style and process — not documentation defects. + +Two corollaries: + +- **Search closed issues and the code before claiming nothing tracks this.** A + gap that was fixed yesterday is not a gap. `gh issue list --state all` and read + the current source, not just the doc. +- **Follow cross-references before concluding something is undocumented.** A page + that links onward to a deeper treatment has documented the thing. + +Recent calibration — findings that should never have been filed: an issue against +a doc that said a value is "not currently persisted across a process restart" +(that sentence *is* the documentation); an issue against "No on-call rotation," +an honest statement of current state; an issue about a topic the page cross-linked +to `security-model.md`, which covered it in more depth than the finding asked for; +and an issue claiming no tracking issue existed when one had closed hours earlier. +Findings that were correct all shared one trait: the missing thing had **literally +zero mentions** anywhere in the repo. Verify that before you file. + ## Workflow 1. Read the kick message for any specific documentation tasks diff --git a/src/pkg/policies/guide_command_verification_test.go b/src/pkg/policies/guide_command_verification_test.go index 857d3fc6e..31f7f503a 100644 --- a/src/pkg/policies/guide_command_verification_test.go +++ b/src/pkg/policies/guide_command_verification_test.go @@ -58,3 +58,53 @@ func TestGuidePoliciesRequireCommandVerification(t *testing.T) { }) } } + +// TestGuideFilingPoliciesRequireGapVerification asserts that every guide policy +// which can record a finding carries the "documented limitation is not a gap" +// guardrail, in both the source tree and the embedded defaults. guide.md is +// excluded: it never files. +func TestGuideFilingPoliciesRequireGapVerification(t *testing.T) { + t.Parallel() + + policyNames := []string{ + "guide-advisory.md", + "guide-issues.md", + "guide-holdgated.md", + "guide-full.md", + } + required := [][]byte{ + []byte("## Before Filing a Finding (MANDATORY)"), + []byte("A documented limitation is not a documentation gap"), + []byte("cross-links to"), + []byte("would a reader who actually hit this situation find the"), + []byte("Search closed issues and the code before claiming nothing tracks this"), + []byte("Follow cross-references before concluding something is undocumented"), + []byte("zero mentions"), + } + + for _, name := range policyNames { + name := name + t.Run(name, func(t *testing.T) { + t.Parallel() + + embedded, err := DefaultPolicies.ReadFile(path.Join("defaults", name)) + if err != nil { + t.Fatalf("read embedded policy: %v", err) + } + source, err := os.ReadFile(filepath.Join("..", "..", "policies", name)) + if err != nil { + t.Fatalf("read source policy: %v", err) + } + for sourceName, policy := range map[string][]byte{ + "embedded": embedded, + "source": source, + } { + for _, marker := range required { + if !bytes.Contains(policy, marker) { + t.Errorf("%s policy is missing gap-verification guardrail %q", sourceName, marker) + } + } + } + }) + } +} diff --git a/src/policies/guide-advisory.md b/src/policies/guide-advisory.md index b9807d718..8568722ce 100644 --- a/src/policies/guide-advisory.md +++ b/src/policies/guide-advisory.md @@ -49,6 +49,36 @@ bd update --set-metadata file="" Finding types: `docs`, `onboarding`, `architecture`, `api`, `contributing` +## Before Filing a Finding (MANDATORY) + +**A documented limitation is not a documentation gap.** Before you file anything, +grep the repo for the thing you claim is missing — including every doc the page +cross-links to. If the text is already there and it is accurate, your finding is +at most "this could be more prominent." That is polish, not a defect, and it does +not get an issue. + +The test is simple: **would a reader who actually hit this situation find the +answer?** If yes, the docs are working. Which file the answer lives in, whether +it is phrased the way you would phrase it, and whether a tracking issue exists +are matters of style and process — not documentation defects. + +Two corollaries: + +- **Search closed issues and the code before claiming nothing tracks this.** A + gap that was fixed yesterday is not a gap. `gh issue list --state all` and read + the current source, not just the doc. +- **Follow cross-references before concluding something is undocumented.** A page + that links onward to a deeper treatment has documented the thing. + +Recent calibration — findings that should never have been filed: an issue against +a doc that said a value is "not currently persisted across a process restart" +(that sentence *is* the documentation); an issue against "No on-call rotation," +an honest statement of current state; an issue about a topic the page cross-linked +to `security-model.md`, which covered it in more depth than the finding asked for; +and an issue claiming no tracking issue existed when one had closed hours earlier. +Findings that were correct all shared one trait: the missing thing had **literally +zero mentions** anywhere in the repo. Verify that before you file. + ## Workflow 1. Read the kick message for any specific documentation tasks diff --git a/src/policies/guide-full.md b/src/policies/guide-full.md index 7e5188e31..e695591fe 100644 --- a/src/policies/guide-full.md +++ b/src/policies/guide-full.md @@ -59,6 +59,36 @@ bd create --title "" \ --type advisory --priority <0-3> --actor guide --external-ref "" ``` +## Before Filing a Finding (MANDATORY) + +**A documented limitation is not a documentation gap.** Before you file anything, +grep the repo for the thing you claim is missing — including every doc the page +cross-links to. If the text is already there and it is accurate, your finding is +at most "this could be more prominent." That is polish, not a defect, and it does +not get an issue. + +The test is simple: **would a reader who actually hit this situation find the +answer?** If yes, the docs are working. Which file the answer lives in, whether +it is phrased the way you would phrase it, and whether a tracking issue exists +are matters of style and process — not documentation defects. + +Two corollaries: + +- **Search closed issues and the code before claiming nothing tracks this.** A + gap that was fixed yesterday is not a gap. `gh issue list --state all` and read + the current source, not just the doc. +- **Follow cross-references before concluding something is undocumented.** A page + that links onward to a deeper treatment has documented the thing. + +Recent calibration — findings that should never have been filed: an issue against +a doc that said a value is "not currently persisted across a process restart" +(that sentence *is* the documentation); an issue against "No on-call rotation," +an honest statement of current state; an issue about a topic the page cross-linked +to `security-model.md`, which covered it in more depth than the finding asked for; +and an issue claiming no tracking issue existed when one had closed hours earlier. +Findings that were correct all shared one trait: the missing thing had **literally +zero mentions** anywhere in the repo. Verify that before you file. + ## Workflow 1. Read the kick message diff --git a/src/policies/guide-holdgated.md b/src/policies/guide-holdgated.md index f45bc1371..c62a6ccf6 100644 --- a/src/policies/guide-holdgated.md +++ b/src/policies/guide-holdgated.md @@ -60,6 +60,36 @@ bd create --title "" \ --type advisory --priority <0-3> --actor guide --external-ref "" ``` +## Before Filing a Finding (MANDATORY) + +**A documented limitation is not a documentation gap.** Before you file anything, +grep the repo for the thing you claim is missing — including every doc the page +cross-links to. If the text is already there and it is accurate, your finding is +at most "this could be more prominent." That is polish, not a defect, and it does +not get an issue. + +The test is simple: **would a reader who actually hit this situation find the +answer?** If yes, the docs are working. Which file the answer lives in, whether +it is phrased the way you would phrase it, and whether a tracking issue exists +are matters of style and process — not documentation defects. + +Two corollaries: + +- **Search closed issues and the code before claiming nothing tracks this.** A + gap that was fixed yesterday is not a gap. `gh issue list --state all` and read + the current source, not just the doc. +- **Follow cross-references before concluding something is undocumented.** A page + that links onward to a deeper treatment has documented the thing. + +Recent calibration — findings that should never have been filed: an issue against +a doc that said a value is "not currently persisted across a process restart" +(that sentence *is* the documentation); an issue against "No on-call rotation," +an honest statement of current state; an issue about a topic the page cross-linked +to `security-model.md`, which covered it in more depth than the finding asked for; +and an issue claiming no tracking issue existed when one had closed hours earlier. +Findings that were correct all shared one trait: the missing thing had **literally +zero mentions** anywhere in the repo. Verify that before you file. + ## Workflow 1. Read the kick message diff --git a/src/policies/guide-issues.md b/src/policies/guide-issues.md index 98e09e08b..5a3074b1c 100644 --- a/src/policies/guide-issues.md +++ b/src/policies/guide-issues.md @@ -63,6 +63,36 @@ bd create --title "" \ Priority: 0 (no README/build instructions), 1 (missing setup/stale arch docs), 2 (missing contributor guide), 3 (typos/style) +## Before Filing a Finding (MANDATORY) + +**A documented limitation is not a documentation gap.** Before you file anything, +grep the repo for the thing you claim is missing — including every doc the page +cross-links to. If the text is already there and it is accurate, your finding is +at most "this could be more prominent." That is polish, not a defect, and it does +not get an issue. + +The test is simple: **would a reader who actually hit this situation find the +answer?** If yes, the docs are working. Which file the answer lives in, whether +it is phrased the way you would phrase it, and whether a tracking issue exists +are matters of style and process — not documentation defects. + +Two corollaries: + +- **Search closed issues and the code before claiming nothing tracks this.** A + gap that was fixed yesterday is not a gap. `gh issue list --state all` and read + the current source, not just the doc. +- **Follow cross-references before concluding something is undocumented.** A page + that links onward to a deeper treatment has documented the thing. + +Recent calibration — findings that should never have been filed: an issue against +a doc that said a value is "not currently persisted across a process restart" +(that sentence *is* the documentation); an issue against "No on-call rotation," +an honest statement of current state; an issue about a topic the page cross-linked +to `security-model.md`, which covered it in more depth than the finding asked for; +and an issue claiming no tracking issue existed when one had closed hours earlier. +Findings that were correct all shared one trait: the missing thing had **literally +zero mentions** anywhere in the repo. Verify that before you file. + ## Workflow 1. Read the kick message for any specific documentation tasks From 9e6629e434b224275c0cba4d5197de624d0524ff Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Tue, 1 Sep 2026 09:14:37 -0400 Subject: [PATCH 003/190] fix(config): poll for reloaded org instead of reload counter in flaky watcher test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestWatcher_ReloadsOnChange failed the required v2 Tests gate on PRs that do not touch pkg/config (e.g. #5440, run 33488167240, job `test (rest 2/3)`) with: watcher_test.go:62: expected org = "updated-org" after reload, got "" Root cause is a race in the test, not in the watcher. The onChange callback increments reloadCount before it stores lastOrg: reloadCount.Add(1) lastOrg.Store(cfg.Project.Org) while the wait loop exited as soon as reloadCount > 0 and then immediately read lastOrg. When the poll observes the counter in the window between those two statements, lastOrg is still unset and the type assertion yields "" — the exact reported failure. The window is tiny locally but is readily hit under -race on a loaded CI runner. Note this is NOT a partial/truncated read of a non-atomic os.WriteFile as first suspected: every truncation of the config fails to parse or fails validation ("project.org is required"), so onChange is never invoked and no empty org can ever be stored. The observed "" is the never-stored case. Fix: poll for the observable outcome (lastOrg == "updated-org") rather than for the reload counter, with a generous deadline and a tighter poll interval. The assertion is unchanged in strength — the test still proves the watcher reloads and delivers the new content, and still requires reloadCount > 0. Verification, all under -short -race: - Reproduced deterministically by injecting a stall between the increment and the store (modelling a scheduler preemption): 10/10 failures with the identical message, 0/25 after the fix; still 0/10 with a 3s stall. - Real test: 200/200 pass; all three TestWatcher_* 100x under 12-way CPU saturation: pass. Fixes #5448 Signed-off-by: Andrew Anderson --- src/pkg/config/watcher_test.go | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/pkg/config/watcher_test.go b/src/pkg/config/watcher_test.go index fb215282b..9d182181f 100644 --- a/src/pkg/config/watcher_test.go +++ b/src/pkg/config/watcher_test.go @@ -43,23 +43,34 @@ func TestWatcher_ReloadsOnChange(t *testing.T) { t.Fatal(err) } - // Wait for debounce + reload - const waitForReload = 2 * time.Second + // Wait for the debounced reload to deliver the *new* content. + // + // Poll on the observable outcome (lastOrg) rather than on reloadCount. + // The callback increments reloadCount before it stores lastOrg, so a + // loop that exits on reloadCount > 0 can observe the counter in the + // window between those two statements and then read a still-unset + // lastOrg — yielding org == "" and a spurious failure. That window is + // tiny locally but is readily hit under -race on a loaded CI runner. + const waitForReload = 10 * time.Second + const pollInterval = 10 * time.Millisecond deadline := time.Now().Add(waitForReload) + var org string for time.Now().Before(deadline) { - if reloadCount.Load() > 0 { + if v, ok := lastOrg.Load().(string); ok && v == "updated-org" { + org = v break } - time.Sleep(50 * time.Millisecond) + time.Sleep(pollInterval) } if reloadCount.Load() == 0 { t.Fatal("expected at least one reload after file change") } - org, ok := lastOrg.Load().(string) - if !ok || org != "updated-org" { - t.Errorf("expected org = %q after reload, got %q", "updated-org", org) + if org != "updated-org" { + last, _ := lastOrg.Load().(string) + t.Errorf("expected org = %q after reload, got %q (reloads=%d)", + "updated-org", last, reloadCount.Load()) } } From d46c4894478340bdddb28324179d311f912bf980 Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Tue, 1 Sep 2026 09:22:20 -0400 Subject: [PATCH 004/190] =?UTF-8?q?=F0=9F=90=9B=20knowledge:=20wire=20cura?= =?UTF-8?q?tor=20schedule=20to=20promotion,=20opt-in=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit knowledge.curator.schedule was parsed, defaulted to "daily", and never read. Setting it in hive.yaml did nothing (#5430). Wiring it as-is would have been unsafe. applyDefaults stamped "daily" onto every hive that omitted the key, so a naive fix would have started scheduled auto-promotion fleet-wide on hives that never opted in — and auto-promotion copies facts into a higher-precedence knowledge layer with no human review. Upgrading must not silently begin mutating the org layer. So the wiring is opt-in: - KnowledgeCurator/CuratorConfig gain Enabled *bool, defaulting to FALSE when absent. The pointer is what lets the config layer tell "absent" from "explicitly false" — the same shape BeadSynthesizerConfig already uses, but with the opposite default, because promotion writes without review where synthesis does not. - applyDefaults no longer stamps a schedule onto a hive that did not opt in, so the effective config stops advertising a cadence that will not run. The threshold is still defaulted: it is a ceiling on what may be promoted, useful even while idle. - PromotionScheduler drives Promoter.AutoPromoteCandidates on the configured cadence, gated on IsEnabled(). Disabled hives start no ticker and make no calls, so the common case costs nothing. - A schedule set without the opt-in logs a one-line notice naming the key to set, rather than failing silently the way the original bug did. Interval reuses ParseSynthSchedule rather than adding a second parser: the curator's documented vocabulary is "daily", already in that parser's daily/hourly switch over named hour constants. Also fixes an adjacent gap found while wiring: cfg.Knowledge.Curator was never copied into knowledge.KnowledgeConfig at any production call site, so NewPromoter always received a zero CuratorConfig and AutoPromoteThreshold never reached the promoter outside tests. It is now passed through, which is what makes the threshold gate real. Promotion semantics are untouched — the scheduler decides when to sweep and logs what it wrote; which facts qualify remains AutoPromoteCandidates' business, threshold and verified-status gates included. Every promoted fact is logged at INFO with slug and layers, so background writes are auditable. Tests assert observable behaviour via fake wiki layers, counting real ingests rather than flags: an unconfigured hive performs no list and no ingest; enabled:false likewise; an opted-in hive promotes; a 0.42-confidence fact under a 0.90 threshold does not; an unverified fact does not. The opt-in test is mutation-verified — removing the IsEnabled() guard makes it fail. Fixes #5430 Signed-off-by: Andrew Anderson --- src/cmd/hive/main.go | 42 +++ src/hive.yaml.example | 11 +- src/pkg/config/config.go | 30 +- src/pkg/config/config_test.go | 17 +- src/pkg/config/curator_optin_test.go | 79 ++++++ src/pkg/knowledge/api.go | 6 + src/pkg/knowledge/curator_schedule.go | 203 ++++++++++++++ src/pkg/knowledge/curator_schedule_test.go | 312 +++++++++++++++++++++ src/pkg/knowledge/types.go | 46 +-- 9 files changed, 725 insertions(+), 21 deletions(-) create mode 100644 src/pkg/config/curator_optin_test.go create mode 100644 src/pkg/knowledge/curator_schedule.go create mode 100644 src/pkg/knowledge/curator_schedule_test.go diff --git a/src/cmd/hive/main.go b/src/cmd/hive/main.go index f890a5681..33eae5b29 100644 --- a/src/cmd/hive/main.go +++ b/src/cmd/hive/main.go @@ -2248,9 +2248,14 @@ func main() { var knowledgeAPI *knowledge.KnowledgeAPI if cfg.Knowledge.Enabled { layers := convertKnowledgeLayers(cfg.Knowledge.Layers) + // The curator block was previously dropped here, so NewPromoter always + // received a zero CuratorConfig and AutoPromoteThreshold never reached + // the promoter in production. Passing it through is what makes the + // threshold gate real for the scheduled sweep (#5430). knowledgeAPI = knowledge.NewKnowledgeAPI(layers, knowledge.KnowledgeConfig{ Enabled: cfg.Knowledge.Enabled, Engine: cfg.Knowledge.Engine, + Curator: curatorConfigFromHive(cfg.Knowledge.Curator), }, logger) } @@ -2458,6 +2463,27 @@ func main() { } } + // Scheduled knowledge promotion (#5430). knowledge.curator.schedule used to + // be parsed, defaulted to "daily", and never read. It now drives a real + // sweep — but ONLY when knowledge.curator.enabled is explicitly true. + // StartBackground is a no-op otherwise, and logs a notice if a schedule was + // configured without the opt-in so the mismatch is visible rather than + // silent. Do not replace the IsEnabled() guard with a schedule check: that + // would enable unreviewed promotion on every hive that omits the key. + if knowledgeAPI != nil && cfg.Knowledge.Curator.IsEnabled() { + promotionScheduler := knowledge.NewPromotionScheduler( + knowledgeAPI.Promoter(), + curatorConfigFromHive(cfg.Knowledge.Curator), + logger, + ) + promotionScheduler.StartBackground(ctx) + } else if cfg.Knowledge.Curator.Schedule != "" { + logger.Info("knowledge.curator.schedule is set but scheduled promotion is disabled", + "schedule", cfg.Knowledge.Curator.Schedule, + "hint", "set knowledge.curator.enabled: true to opt in", + ) + } + // Open the graph store in a background goroutine. NewGraphStore acquires // a SQLite file lock that blocks if the old pod still holds it. Deferring // this lets the HTTP server start so the readiness probe passes, which @@ -7038,6 +7064,22 @@ func convertKnowledgeLayers(cfgLayers []config.KnowledgeLayer) []knowledge.Layer return layers } +// curatorConfigFromHive maps the hive.yaml curator block onto the knowledge +// package's own config. Enabled is carried across as a pointer so "absent" +// stays distinguishable from "explicitly false" — the scheduled promotion loop +// treats absent as OFF, and flattening it to a bool here would quietly turn +// unreviewed promotion on fleet-wide (#5430). +func curatorConfigFromHive(c config.KnowledgeCurator) knowledge.CuratorConfig { + return knowledge.CuratorConfig{ + Enabled: c.Enabled, + Schedule: c.Schedule, + ExtractFrom: c.ExtractFrom, + AutoPromoteThreshold: c.AutoPromoteThreshold, + PromoteFrom: c.PromoteFrom, + PromoteTo: c.PromoteTo, + } +} + // hiveIDFilePath is the persistent file where the Hive ID is stored across restarts. const hiveIDFilePath = "/data/hive-id" diff --git a/src/hive.yaml.example b/src/hive.yaml.example index 745bbabd9..a12641648 100644 --- a/src/hive.yaml.example +++ b/src/hive.yaml.example @@ -553,12 +553,21 @@ knowledge: # url: https://wiki.hive.dev/mcp # Public, read-only # shared: true curator: - schedule: daily # How often to extract facts from merged PRs + # enabled: true # OPT-IN. Absent or false = scheduled + # auto-promotion never runs, even with a + # schedule set below. Auto-promotion copies + # facts into a higher layer with NO human + # review, so it is off unless you ask for it. + schedule: daily # Cadence for the promotion sweep once + # enabled: true. Accepts daily or hourly. + # Ignored entirely while disabled. extract_from: - pr_comments - ci_failures - review_comments auto_promote_threshold: 0.9 # Confidence to auto-promote project→org + # promote_from: project # Source layer (default: project) + # promote_to: org # Target layer (default: org) git_sources: # Remote git repos indexed as knowledge # - name: dakota-skills # Display name for this source # url: https://github.com/projectbluefin/dakota diff --git a/src/pkg/config/config.go b/src/pkg/config/config.go index feb4a20e1..4cc936a4e 100644 --- a/src/pkg/config/config.go +++ b/src/pkg/config/config.go @@ -584,9 +584,31 @@ type KnowledgeLayer struct { } type KnowledgeCurator struct { + // Enabled gates the scheduled auto-promotion loop. It is a pointer so an + // absent key is distinguishable from an explicit `enabled: false`, and it + // defaults to FALSE — unlike BeadSynthesizer, which defaults to true. + // + // The asymmetry is deliberate. Auto-promotion copies facts into a + // higher-precedence knowledge layer with no human review, and `schedule` + // has been parsed-but-unactioned since it was introduced (#5430), so every + // existing hive that set it did so without ever having the loop run. If + // the loop defaulted on, upgrading would silently begin mutating the org + // layer on hives that never opted in. Scheduled promotion is therefore + // opt-in: `schedule` alone does NOT start it. + Enabled *bool `yaml:"enabled,omitempty"` Schedule string `yaml:"schedule"` ExtractFrom []string `yaml:"extract_from"` AutoPromoteThreshold float64 `yaml:"auto_promote_threshold"` + // PromoteFrom / PromoteTo name the source and target layers for the + // scheduled promotion sweep. Empty values fall back to project→org. + PromoteFrom string `yaml:"promote_from,omitempty"` + PromoteTo string `yaml:"promote_to,omitempty"` +} + +// IsEnabled reports whether scheduled auto-promotion is active. Absent (nil) +// means DISABLED — see the Enabled field comment for why this defaults false. +func (k KnowledgeCurator) IsEnabled() bool { + return k.Enabled != nil && *k.Enabled } type KnowledgePrimer struct { @@ -4677,7 +4699,13 @@ func (c *Config) applyDefaults() { if len(c.Knowledge.Primer.Priority) == 0 { c.Knowledge.Primer.Priority = []string{"regression", "gotcha", "test_scaffold", "pattern", "decision"} } - if c.Knowledge.Curator.Schedule == "" { + // Schedule is only defaulted when the curator has been explicitly + // enabled. Defaulting it unconditionally (the pre-#5430 behaviour) was + // harmless while nothing read the field, but now that it drives a + // promotion loop a blanket default would hand every hive a cadence it + // never asked for. The Enabled gate is the real guard; leaving Schedule + // empty on disabled hives keeps the config honest about what will run. + if c.Knowledge.Curator.IsEnabled() && c.Knowledge.Curator.Schedule == "" { c.Knowledge.Curator.Schedule = defaultCuratorSchedule } if c.Knowledge.Curator.AutoPromoteThreshold == 0 { diff --git a/src/pkg/config/config_test.go b/src/pkg/config/config_test.go index 5741ca8d2..950b4e329 100644 --- a/src/pkg/config/config_test.go +++ b/src/pkg/config/config_test.go @@ -936,9 +936,20 @@ func TestApplyDefaults_KnowledgeDefaults(t *testing.T) { if len(cfg.Knowledge.Primer.Priority) == 0 { t.Error("expected default priority") } - if cfg.Knowledge.Curator.Schedule != defaultCuratorSchedule { - t.Errorf("schedule = %q", cfg.Knowledge.Curator.Schedule) - } + // Curator schedule is deliberately NOT defaulted here. Since #5430 wired + // the schedule to a real promotion loop, stamping "daily" onto a hive that + // never set knowledge.curator.enabled would advertise a cadence that (a) + // will not run, and (b) must not run — auto-promotion writes into a higher + // knowledge layer without review. The default applies only once opted in; + // see TestCuratorEnabledGetsScheduleDefault in curator_optin_test.go. + if cfg.Knowledge.Curator.IsEnabled() { + t.Error("curator must default to disabled") + } + if cfg.Knowledge.Curator.Schedule != "" { + t.Errorf("schedule = %q, want empty on a hive that did not opt in", cfg.Knowledge.Curator.Schedule) + } + // The threshold is still defaulted unconditionally: it is a ceiling on what + // may ever be promoted, so a value is useful even when nothing is running. if cfg.Knowledge.Curator.AutoPromoteThreshold != defaultPromoteThreshold { t.Errorf("threshold = %f", cfg.Knowledge.Curator.AutoPromoteThreshold) } diff --git a/src/pkg/config/curator_optin_test.go b/src/pkg/config/curator_optin_test.go new file mode 100644 index 000000000..74e2a07e8 --- /dev/null +++ b/src/pkg/config/curator_optin_test.go @@ -0,0 +1,79 @@ +package config + +import "testing" + +// TestCuratorDisabledByDefault pins the opt-in contract for #5430 at the config +// layer: a knowledge block that says nothing about the curator must report +// disabled, so the promotion scheduler never starts on an existing hive that is +// merely upgraded. +func TestCuratorDisabledByDefault(t *testing.T) { + cfg := &Config{} + cfg.Knowledge.Enabled = true + cfg.applyDefaults() + + if cfg.Knowledge.Curator.IsEnabled() { + t.Fatal("curator must be disabled when knowledge.curator.enabled is absent") + } + // The pre-#5430 code stamped "daily" here unconditionally. Leaving it empty + // on a disabled hive keeps the effective config honest about what runs. + if cfg.Knowledge.Curator.Schedule != "" { + t.Errorf("schedule defaulted to %q on a hive that never opted in; want empty", + cfg.Knowledge.Curator.Schedule) + } +} + +// TestCuratorExplicitFalseStaysDisabled covers the third state the pointer +// makes representable — set, and set to off. +func TestCuratorExplicitFalseStaysDisabled(t *testing.T) { + off := false + cfg := &Config{} + cfg.Knowledge.Enabled = true + cfg.Knowledge.Curator.Enabled = &off + cfg.Knowledge.Curator.Schedule = "hourly" + cfg.applyDefaults() + + if cfg.Knowledge.Curator.IsEnabled() { + t.Error("enabled:false must stay disabled") + } + // An explicitly configured schedule is preserved verbatim; it simply is not + // acted on while disabled. + if cfg.Knowledge.Curator.Schedule != "hourly" { + t.Errorf("schedule = %q, want it preserved as \"hourly\"", cfg.Knowledge.Curator.Schedule) + } +} + +// TestCuratorEnabledGetsScheduleDefault shows the default cadence is still +// applied — but only once an operator has opted in. +func TestCuratorEnabledGetsScheduleDefault(t *testing.T) { + on := true + cfg := &Config{} + cfg.Knowledge.Enabled = true + cfg.Knowledge.Curator.Enabled = &on + cfg.applyDefaults() + + if !cfg.Knowledge.Curator.IsEnabled() { + t.Fatal("enabled:true must report enabled") + } + if cfg.Knowledge.Curator.Schedule != defaultCuratorSchedule { + t.Errorf("schedule = %q, want %q", cfg.Knowledge.Curator.Schedule, defaultCuratorSchedule) + } + if cfg.Knowledge.Curator.AutoPromoteThreshold != defaultPromoteThreshold { + t.Errorf("threshold = %v, want %v", + cfg.Knowledge.Curator.AutoPromoteThreshold, defaultPromoteThreshold) + } +} + +// TestCuratorEnabledPreservesExplicitSchedule ensures the default never +// overwrites an operator's choice. +func TestCuratorEnabledPreservesExplicitSchedule(t *testing.T) { + on := true + cfg := &Config{} + cfg.Knowledge.Enabled = true + cfg.Knowledge.Curator.Enabled = &on + cfg.Knowledge.Curator.Schedule = "hourly" + cfg.applyDefaults() + + if cfg.Knowledge.Curator.Schedule != "hourly" { + t.Errorf("schedule = %q, want \"hourly\"", cfg.Knowledge.Curator.Schedule) + } +} diff --git a/src/pkg/knowledge/api.go b/src/pkg/knowledge/api.go index a0139e0fe..d0eee3697 100644 --- a/src/pkg/knowledge/api.go +++ b/src/pkg/knowledge/api.go @@ -361,6 +361,12 @@ func (k *KnowledgeAPI) DeleteFact(ctx context.Context, layer LayerType, slug str return nil } +// Promoter exposes the layer promoter so the scheduled promotion loop can be +// built over the same clients the dashboard's manual promote path uses. +func (k *KnowledgeAPI) Promoter() *Promoter { + return k.promoter +} + // PromoteFact promotes a fact from one layer to another (upward only). func (k *KnowledgeAPI) PromoteFact(ctx context.Context, req PromoteRequest) PromoteResult { return k.promoter.Promote(ctx, req) diff --git a/src/pkg/knowledge/curator_schedule.go b/src/pkg/knowledge/curator_schedule.go new file mode 100644 index 000000000..a4cb092d9 --- /dev/null +++ b/src/pkg/knowledge/curator_schedule.go @@ -0,0 +1,203 @@ +package knowledge + +import ( + "context" + "log/slog" + "sync" + "time" +) + +// Layer defaults for the scheduled promotion sweep. project→org is the +// promotion documented in hive.yaml.example ("Confidence to auto-promote +// project→org") and is the only pair the shipped example config implies. +const ( + defaultPromoteFromLayer = LayerProject + defaultPromoteToLayer = LayerOrg +) + +// PromotionScheduler drives Promoter.AutoPromoteCandidates on the cadence set +// by knowledge.curator.schedule. +// +// It exists because that schedule field was parsed, defaulted and never read +// (#5430). Wiring it naively would have been a behaviour change for every +// existing deployment, since applyDefaults stamped "daily" onto any hive that +// omitted the key — so the scheduler is gated on CuratorConfig.IsEnabled(), +// which is false unless an operator writes `enabled: true`. A configured +// schedule on a hive that never opted in still does nothing, by design. +// +// The scheduler adds no promotion semantics: it decides WHEN to sweep and +// logs WHAT was swept. Which facts qualify remains entirely +// AutoPromoteCandidates' business, including the AutoPromoteThreshold gate. +type PromotionScheduler struct { + promoter *Promoter + config CuratorConfig + logger *slog.Logger + + from LayerType + to LayerType + + mu sync.Mutex + cancel context.CancelFunc + running bool +} + +// NewPromotionScheduler builds a scheduler over an existing promoter. It never +// starts anything on its own — callers must invoke Start/StartBackground, and +// those are no-ops unless the config opts in. +func NewPromotionScheduler(promoter *Promoter, config CuratorConfig, logger *slog.Logger) *PromotionScheduler { + from := LayerType(config.PromoteFrom) + if from == "" { + from = defaultPromoteFromLayer + } + to := LayerType(config.PromoteTo) + if to == "" { + to = defaultPromoteToLayer + } + return &PromotionScheduler{ + promoter: promoter, + config: config, + logger: logger, + from: from, + to: to, + } +} + +// Interval returns the tick period for the configured schedule. It reuses +// ParseSynthSchedule rather than introducing a second parser: the curator's +// documented vocabulary is "daily" (defaultCuratorSchedule) and the +// synthesizer's parser already maps daily/hourly onto named hour constants, so +// the two agree with no new values needed. +func (s *PromotionScheduler) Interval() time.Duration { + return ParseSynthSchedule(s.config.Schedule) +} + +// Start runs the promotion loop until ctx is cancelled. If the curator is not +// explicitly enabled it returns immediately, having done no work and started +// no ticker — the disabled path costs nothing per tick because there is no +// tick. +func (s *PromotionScheduler) Start(ctx context.Context) { + if !s.config.IsEnabled() { + // Only worth a line when an operator set a cadence that will not run; + // silence would reproduce exactly the confusion #5430 describes. + if s.config.Schedule != "" && s.logger != nil { + s.logger.Info("scheduled knowledge promotion is disabled; configured schedule will not run", + "schedule", s.config.Schedule, + "hint", "set knowledge.curator.enabled: true to opt in", + ) + } + return + } + if s.promoter == nil { + return + } + + interval := s.Interval() + s.logger.Info("scheduled knowledge promotion started", + "schedule", s.config.Schedule, + "interval", interval, + "from", s.from, + "to", s.to, + "threshold", s.config.AutoPromoteThreshold, + ) + + ticker := time.NewTicker(interval) + defer ticker.Stop() + + s.RunOnce(ctx) + + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + s.RunOnce(ctx) + } + } +} + +// RunOnce performs a single promotion sweep. It is exported so the dashboard +// and tests can trigger a sweep without waiting out an interval; it still +// honours the enable gate, so it cannot be used to bypass opt-in. +func (s *PromotionScheduler) RunOnce(ctx context.Context) { + if !s.config.IsEnabled() || s.promoter == nil { + return + } + + candidates, err := s.promoter.AutoPromoteCandidates(ctx, s.from, s.to) + if err != nil { + s.logger.Warn("scheduled promotion: listing candidates failed", + "from", s.from, "to", s.to, "error", err) + return + } + if len(candidates) == 0 { + return + } + + var promoted, failed int + for _, req := range candidates { + select { + case <-ctx.Done(): + return + default: + } + result := s.promoter.Promote(ctx, req) + if result.Success { + promoted++ + // Per-fact INFO: a background job that writes into a knowledge + // layer must name what it wrote and where it came from, or an + // operator has no way to audit or undo it. + s.logger.Info("scheduled promotion: fact promoted", + "slug", req.Slug, "from", req.FromLayer, "to", req.ToLayer, "reason", req.Reason) + continue + } + failed++ + s.logger.Warn("scheduled promotion: fact promotion failed", + "slug", req.Slug, "from", req.FromLayer, "to", req.ToLayer, "error", result.Error) + } + + s.logger.Info("scheduled promotion sweep complete", + "from", s.from, "to", s.to, + "candidates", len(candidates), "promoted", promoted, "failed", failed, + "threshold", s.config.AutoPromoteThreshold, + ) +} + +// StartBackground launches the loop in a goroutine, tracking it so it can be +// stopped and restarted. It is a no-op when the curator is not enabled. +func (s *PromotionScheduler) StartBackground(parent context.Context) { + if !s.config.IsEnabled() { + s.Start(parent) // emits the disabled-with-schedule notice, returns + return + } + s.mu.Lock() + defer s.mu.Unlock() + if s.running { + return + } + ctx, cancel := context.WithCancel(parent) + s.cancel = cancel + s.running = true + go func() { + s.Start(ctx) + s.mu.Lock() + s.running = false + s.mu.Unlock() + }() +} + +// Stop cancels the background loop. +func (s *PromotionScheduler) Stop() { + s.mu.Lock() + defer s.mu.Unlock() + if s.cancel != nil { + s.cancel() + s.cancel = nil + } +} + +// IsRunning reports whether the background loop is active. +func (s *PromotionScheduler) IsRunning() bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.running +} diff --git a/src/pkg/knowledge/curator_schedule_test.go b/src/pkg/knowledge/curator_schedule_test.go new file mode 100644 index 000000000..1702c75a0 --- /dev/null +++ b/src/pkg/knowledge/curator_schedule_test.go @@ -0,0 +1,312 @@ +package knowledge + +import ( + "context" + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" +) + +// promoteProbe stands in for a pair of wiki layers and records what was +// actually written to the TARGET layer. Tests assert on ingests observed here +// rather than on scheduler flags, so a scheduler that "runs" but promotes +// nothing cannot pass, and neither can one that is wired but never ticks. +type promoteProbe struct { + mu sync.Mutex + ingests [][]ExtractedFact + listHits int + + source *httptest.Server + target *httptest.Server +} + +func (p *promoteProbe) close() { + p.source.Close() + p.target.Close() +} + +func (p *promoteProbe) ingestCount() int { + p.mu.Lock() + defer p.mu.Unlock() + return len(p.ingests) +} + +func (p *promoteProbe) listCount() int { + p.mu.Lock() + defer p.mu.Unlock() + return p.listHits +} + +// newPromoteProbe serves one page at the given confidence/status from the +// source layer and accepts ingests at the target layer. +func newPromoteProbe(slug string, confidence float64, status string) *promoteProbe { + p := &promoteProbe{} + + p.source = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + // ListPages GETs /api/pages exactly; ReadPage GETs /api/pages/. + // Match the list path exactly so the read is not swallowed by a prefix. + if r.URL.Path == "/api/pages" { + p.mu.Lock() + p.listHits++ + p.mu.Unlock() + _ = json.NewEncoder(w).Encode(searchResponse{ + Results: []searchResult{{ + Slug: slug, + Title: "scheduled promotion probe", + Type: "gotcha", + Status: status, + Confidence: confidence, + }}, + }) + return + } + _ = json.NewEncoder(w).Encode(pageResponse{ + Slug: slug, + Title: "scheduled promotion probe", + Body: "probe body", + Type: "gotcha", + Status: status, + Confidence: confidence, + }) + })) + + p.target = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + var facts []ExtractedFact + _ = json.Unmarshal(body, &facts) + p.mu.Lock() + p.ingests = append(p.ingests, facts) + p.mu.Unlock() + w.WriteHeader(http.StatusCreated) + })) + + return p +} + +func (p *promoteProbe) promoter(cfg CuratorConfig) *Promoter { + return NewPromoter([]LayerConfig{ + {Type: LayerProject, URL: p.source.URL}, + {Type: LayerOrg, URL: p.target.URL}, + }, cfg, slog.New(slog.NewTextHandler(io.Discard, nil))) +} + +func boolPtr(b bool) *bool { return &b } + +func schedTestLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +// TestScheduledPromotionDisabledByDefaultDoesNotRun is THE guard test for +// #5430. An unconfigured hive — one that never set knowledge.curator.enabled — +// must perform no promotion at all, even though applyDefaults historically +// stamped schedule: daily onto it. +// +// This test fails if the opt-in guard is removed: deleting the +// `if !s.config.IsEnabled()` early return from Start/RunOnce makes RunOnce +// list candidates and ingest the 0.95-confidence page, so both ingestCount and +// listCount become non-zero and both assertions below fail. +func TestScheduledPromotionDisabledByDefaultDoesNotRun(t *testing.T) { + probe := newPromoteProbe("guard-fact", 0.95, "verified") + defer probe.close() + + // Exactly what a hive that never opted in looks like: a schedule present + // (the historical default) but no Enabled key. + cfg := CuratorConfig{Schedule: "daily", AutoPromoteThreshold: 0.9} + if cfg.IsEnabled() { + t.Fatal("CuratorConfig with no Enabled key must report disabled") + } + + s := NewPromotionScheduler(probe.promoter(cfg), cfg, schedTestLogger()) + + // Start must return immediately rather than blocking on a ticker. + done := make(chan struct{}) + go func() { + s.Start(context.Background()) + close(done) + }() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("Start blocked on a disabled curator: the opt-in guard is not short-circuiting") + } + + // And a direct sweep must also refuse. + s.RunOnce(context.Background()) + + if got := probe.ingestCount(); got != 0 { + t.Errorf("disabled curator promoted %d fact batches; want 0 — unconfigured hives must never auto-promote", got) + } + if got := probe.listCount(); got != 0 { + t.Errorf("disabled curator made %d candidate-list calls; want 0 — the disabled path must cost nothing", got) + } + if s.IsRunning() { + t.Error("disabled curator reports a running loop") + } +} + +// TestScheduledPromotionExplicitFalseDoesNotRun covers the third state the +// pointer makes expressible: explicitly opted out. +func TestScheduledPromotionExplicitFalseDoesNotRun(t *testing.T) { + probe := newPromoteProbe("guard-fact", 0.99, "verified") + defer probe.close() + + cfg := CuratorConfig{Enabled: boolPtr(false), Schedule: "hourly", AutoPromoteThreshold: 0.5} + s := NewPromotionScheduler(probe.promoter(cfg), cfg, schedTestLogger()) + s.RunOnce(context.Background()) + + if got := probe.ingestCount(); got != 0 { + t.Errorf("enabled:false promoted %d batches; want 0", got) + } +} + +// TestScheduledPromotionEnabledPromotes proves the wiring actually works — +// that a hive which opts in gets facts promoted into the target layer. +func TestScheduledPromotionEnabledPromotes(t *testing.T) { + probe := newPromoteProbe("wired-fact", 0.95, "verified") + defer probe.close() + + cfg := CuratorConfig{Enabled: boolPtr(true), Schedule: "daily", AutoPromoteThreshold: 0.9} + s := NewPromotionScheduler(probe.promoter(cfg), cfg, schedTestLogger()) + s.RunOnce(context.Background()) + + if got := probe.ingestCount(); got != 1 { + t.Fatalf("enabled curator ingested %d batches; want 1", got) + } + probe.mu.Lock() + facts := probe.ingests[0] + probe.mu.Unlock() + if len(facts) != 1 { + t.Fatalf("ingested %d facts; want 1", len(facts)) + } + if facts[0].Title != "scheduled promotion probe" { + t.Errorf("promoted the wrong fact: title = %q", facts[0].Title) + } +} + +// TestScheduledPromotionRespectsThreshold proves the scheduler did not bypass +// the existing AutoPromoteThreshold gate: same opted-in config, a fact below +// the bar, and nothing may be written. +func TestScheduledPromotionRespectsThreshold(t *testing.T) { + probe := newPromoteProbe("low-confidence-fact", 0.42, "verified") + defer probe.close() + + cfg := CuratorConfig{Enabled: boolPtr(true), Schedule: "daily", AutoPromoteThreshold: 0.9} + s := NewPromotionScheduler(probe.promoter(cfg), cfg, schedTestLogger()) + s.RunOnce(context.Background()) + + if got := probe.listCount(); got == 0 { + t.Fatal("enabled curator never listed candidates; the test is not exercising the gate") + } + if got := probe.ingestCount(); got != 0 { + t.Errorf("promoted %d batches for a 0.42-confidence fact under a 0.90 threshold; want 0", got) + } +} + +// TestScheduledPromotionUnverifiedNotPromoted pins the other half of the +// existing gate: confidence alone is not sufficient, status must be verified. +func TestScheduledPromotionUnverifiedNotPromoted(t *testing.T) { + probe := newPromoteProbe("draft-fact", 0.99, "draft") + defer probe.close() + + cfg := CuratorConfig{Enabled: boolPtr(true), Schedule: "daily", AutoPromoteThreshold: 0.9} + s := NewPromotionScheduler(probe.promoter(cfg), cfg, schedTestLogger()) + s.RunOnce(context.Background()) + + if got := probe.ingestCount(); got != 0 { + t.Errorf("promoted %d batches for an unverified fact; want 0", got) + } +} + +// TestScheduledPromotionIntervalMatchesSchedule asserts the configured cadence +// maps to the expected period — a "daily" hive must not tick hourly. +func TestScheduledPromotionIntervalMatchesSchedule(t *testing.T) { + tests := []struct { + schedule string + want time.Duration + }{ + {"daily", time.Duration(beadSynthDailyScheduleHours) * time.Hour}, + {"hourly", time.Duration(beadSynthDefaultScheduleHours) * time.Hour}, + {"", time.Duration(beadSynthDefaultScheduleHours) * time.Hour}, + } + for _, tt := range tests { + t.Run(tt.schedule, func(t *testing.T) { + cfg := CuratorConfig{Enabled: boolPtr(true), Schedule: tt.schedule} + s := NewPromotionScheduler(nil, cfg, schedTestLogger()) + if got := s.Interval(); got != tt.want { + t.Errorf("Interval() = %v, want %v", got, tt.want) + } + }) + } +} + +// TestScheduledPromotionTicksOnInterval proves a configured schedule produces +// repeated runs rather than a single startup sweep. It drives RunOnce through +// a short ticker instead of waiting an hour, then checks the sweep count grew. +func TestScheduledPromotionTicksOnInterval(t *testing.T) { + probe := newPromoteProbe("tick-fact", 0.95, "verified") + defer probe.close() + + cfg := CuratorConfig{Enabled: boolPtr(true), Schedule: "daily", AutoPromoteThreshold: 0.9} + s := NewPromotionScheduler(probe.promoter(cfg), cfg, schedTestLogger()) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + const sweeps = 3 + for i := 0; i < sweeps; i++ { + s.RunOnce(ctx) + } + if got := probe.ingestCount(); got != sweeps { + t.Errorf("after %d sweeps saw %d ingests; want %d — repeated ticks must each promote", sweeps, got, sweeps) + } +} + +// TestScheduledPromotionDefaultLayers pins the project→org default that +// hive.yaml.example documents. +func TestScheduledPromotionDefaultLayers(t *testing.T) { + s := NewPromotionScheduler(nil, CuratorConfig{Enabled: boolPtr(true)}, schedTestLogger()) + if s.from != LayerProject || s.to != LayerOrg { + t.Errorf("default layers = %s→%s, want %s→%s", s.from, s.to, LayerProject, LayerOrg) + } + + custom := NewPromotionScheduler(nil, CuratorConfig{ + Enabled: boolPtr(true), PromoteFrom: string(LayerPersonal), PromoteTo: string(LayerProject), + }, schedTestLogger()) + if custom.from != LayerPersonal || custom.to != LayerProject { + t.Errorf("configured layers = %s→%s, want %s→%s", custom.from, custom.to, LayerPersonal, LayerProject) + } +} + +// TestScheduledPromotionStopHalts covers lifecycle: an opted-in background +// loop must be stoppable. +func TestScheduledPromotionStopHalts(t *testing.T) { + probe := newPromoteProbe("lifecycle-fact", 0.95, "verified") + defer probe.close() + + cfg := CuratorConfig{Enabled: boolPtr(true), Schedule: "daily", AutoPromoteThreshold: 0.9} + s := NewPromotionScheduler(probe.promoter(cfg), cfg, schedTestLogger()) + + s.StartBackground(context.Background()) + deadline := time.Now().Add(2 * time.Second) + for !s.IsRunning() && time.Now().Before(deadline) { + time.Sleep(5 * time.Millisecond) + } + if !s.IsRunning() { + t.Fatal("StartBackground did not start an opted-in loop") + } + + s.Stop() + deadline = time.Now().Add(2 * time.Second) + for s.IsRunning() && time.Now().Before(deadline) { + time.Sleep(5 * time.Millisecond) + } + if s.IsRunning() { + t.Error("Stop did not halt the loop") + } +} diff --git a/src/pkg/knowledge/types.go b/src/pkg/knowledge/types.go index dbab3f9e4..de0888cf0 100644 --- a/src/pkg/knowledge/types.go +++ b/src/pkg/knowledge/types.go @@ -112,9 +112,23 @@ func (b BeadSynthesizerConfig) IsEnabled() bool { // CuratorConfig controls automated knowledge extraction from merged PRs. type CuratorConfig struct { - Schedule string `yaml:"schedule" json:"schedule"` - ExtractFrom []string `yaml:"extract_from" json:"extract_from"` - AutoPromoteThreshold float64 `yaml:"auto_promote_threshold" json:"auto_promote_threshold"` + // Enabled gates the scheduled auto-promotion loop and defaults to FALSE + // when absent — see config.KnowledgeCurator.Enabled for the reasoning. + // Scheduled promotion writes into a higher layer without human review, so + // it must be opted into explicitly; a bare `schedule` does not start it. + Enabled *bool `yaml:"enabled,omitempty" json:"enabled"` + Schedule string `yaml:"schedule" json:"schedule"` + ExtractFrom []string `yaml:"extract_from" json:"extract_from"` + AutoPromoteThreshold float64 `yaml:"auto_promote_threshold" json:"auto_promote_threshold"` + PromoteFrom string `yaml:"promote_from,omitempty" json:"promote_from,omitempty"` + PromoteTo string `yaml:"promote_to,omitempty" json:"promote_to,omitempty"` +} + +// IsEnabled reports whether scheduled auto-promotion is active. Absent (nil) +// means DISABLED. This is the single guard the promotion scheduler consults; +// removing it turns scheduled, unreviewed layer mutation on fleet-wide. +func (c CuratorConfig) IsEnabled() bool { + return c.Enabled != nil && *c.Enabled } // PrimerConfig controls how facts are selected and injected into agent kicks. @@ -304,19 +318,19 @@ type Question struct { // InceptionState tracks the progress of a Level 1 ideation workflow. type InceptionState struct { - Phase InceptionPhase `json:"phase"` - Mode InceptionMode `json:"mode"` - IdeaText string `json:"idea_text"` - IdeaSlug string `json:"idea_slug"` - RepoURL string `json:"repo_url,omitempty"` - Questions []Question `json:"questions"` - Answers map[string]string `json:"answers"` - FactSlugs []string `json:"fact_slugs"` - StartedAt time.Time `json:"started_at"` - PhaseChangedAt *time.Time `json:"phase_changed_at,omitempty"` - WikiName string `json:"wiki_name,omitempty"` - AutoFactCount int `json:"auto_fact_count,omitempty"` - AutoQuestionCount int `json:"auto_question_count,omitempty"` + Phase InceptionPhase `json:"phase"` + Mode InceptionMode `json:"mode"` + IdeaText string `json:"idea_text"` + IdeaSlug string `json:"idea_slug"` + RepoURL string `json:"repo_url,omitempty"` + Questions []Question `json:"questions"` + Answers map[string]string `json:"answers"` + FactSlugs []string `json:"fact_slugs"` + StartedAt time.Time `json:"started_at"` + PhaseChangedAt *time.Time `json:"phase_changed_at,omitempty"` + WikiName string `json:"wiki_name,omitempty"` + AutoFactCount int `json:"auto_fact_count,omitempty"` + AutoQuestionCount int `json:"auto_question_count,omitempty"` } // ScaffoldFile is a single generated file in the scaffold output. From e17bfa74381ecca99413225787d517722729836c Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Tue, 1 Sep 2026 09:24:43 -0400 Subject: [PATCH 005/190] =?UTF-8?q?=F0=9F=90=9B=20test:=20stop=20the=20age?= =?UTF-8?q?nt-sandbox=20owner-gate=20assertion=20retiring=20itself=20on=20?= =?UTF-8?q?a=20rename=20(#5388)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestF16AgentSandboxToggleIsOwnerOnly anchored on the Go identifier AgentSandboxEnabled and SKIPPED when it was absent, making the skip condition identical to the coupling it guards. A rename of the local struct field — leaving the JSON tag, the config write and the entire wire contract intact, so the toggle stays fully reachable — silently converted an owner-gate security assertion into a passing skip. Anchor on the wire field and the config write instead, which are the actual contract rather than a private naming choice, and fail rather than skip when the toggle genuinely moves. Signed-off-by: Andrew Anderson --- src/pkg/dashboard/f16_owner_gate_test.go | 38 ++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) 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)") From 3532d7561bb5ead33dc76b81bfc2a4f3cc21dd47 Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Tue, 1 Sep 2026 09:32:29 -0400 Subject: [PATCH 006/190] =?UTF-8?q?=F0=9F=90=9B=20test:=20make=20/api/tren?= =?UTF-8?q?ds=20and=20three=20toggles=20assert=20behaviour,=20not=20status?= =?UTF-8?q?=20(#5388)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit api_routes_coverage_test.go asserts Code != 200 at ~30 sites without decoding a body. Four of those are fixed here; the rest are reported. /api/trends passed five query variants and checked only the status, so a handler ignoring range and hours entirely stayed green — demonstrated by replacing the whole switch with a hardcoded 24h window, which left every test in package dashboard passing. The new test seeds sparkline entries at known ages and asserts each parameter selects a different subset, including the maxTrendHours clamp that ?hours=99999 was clearly meant to exercise but never could. The knowledge, bead-synthesizer and budget-ignore toggles set a value and never read it back, so no-op handlers returning 200 passed in both directions. They now assert the state each toggle exists to write. No existing assertion is weakened; every change strictly adds a failure mode. Signed-off-by: Andrew Anderson --- src/pkg/dashboard/api_routes_coverage_test.go | 139 +++++++++++++++++- 1 file changed, 135 insertions(+), 4 deletions(-) diff --git a/src/pkg/dashboard/api_routes_coverage_test.go b/src/pkg/dashboard/api_routes_coverage_test.go index f51ed124d..ce1c285d1 100644 --- a/src/pkg/dashboard/api_routes_coverage_test.go +++ b/src/pkg/dashboard/api_routes_coverage_test.go @@ -1,9 +1,13 @@ package dashboard import ( + "encoding/json" "net/http" "net/http/httptest" + "reflect" + "sort" "testing" + "time" ) // covG2ServeRaw sends a request with a raw (possibly malformed) JSON body and a @@ -35,6 +39,100 @@ func TestCovG2_HistoryTrendsTimeline(t *testing.T) { } } +// TestCovG2_TrendsHonoursRangeAndHours is the #5388 fix for the loop above. +// +// The loop passes five query variants to /api/trends and asserts only that each +// returns 200. It never decodes a body, so it asserts the shape of the response +// (a status code) rather than the property the parameters are supposed to have +// (selecting a time window). Demonstrated: replacing the whole range/hours +// switch in handleTrends with a hardcoded `hours := hoursPerDay` — a handler +// that ignores its parameters completely — left the loop green, and in fact left +// EVERY test in package dashboard green. +// +// This test seeds token-sparkline entries at known ages and asserts each +// parameter selects a provably different subset. Ages are chosen so every +// documented branch of the switch separates from its neighbours: +// +// range=day / no param → 24h → picks up 1h, 12h +// hours=48 → 48h → additionally 30h +// range=week → 168h → additionally 100h +// hours=99999 → clamped to maxTrendHours (720h/30d), so 800h stays +// excluded — that clamp is the reason 99999 is in the +// original list, and it was never actually checked. +func TestCovG2_TrendsHonoursRangeAndHours(t *testing.T) { + s, _ := apiServer(t) + + now := time.Now() + // ageHours → the entry's token count, used as an identifying marker. + ages := []int{1, 12, 30, 100, 800} + entries := make([]TokenSparklineEntry, 0, len(ages)) + for _, age := range ages { + entries = append(entries, TokenSparklineEntry{ + Timestamp: now.Add(-time.Duration(age)*time.Hour - time.Minute).UnixMilli(), + Input: int64(age), + }) + } + s.SeedTokenSparklineHistory(entries) + + // Guard against the seam itself silently doing nothing: if seeding stops + // working, every expectation below collapses to "0 == 0" and this test + // would pass while checking nothing (#5388 anti-vacuity, per #5409). + if got := len(s.TokenSparklineHistory()); got != len(ages) { + t.Fatalf("seed did not take: history has %d entries, want %d — "+ + "the assertions below would be vacuous", got, len(ages)) + } + + // wantAges is the set of seeded entries each query must return, keyed by the + // marker written into Input above. + for _, tc := range []struct { + query string + wantAges []int + }{ + {"", []int{1, 12}}, + {"?range=day", []int{1, 12}}, + {"?hours=48", []int{1, 12, 30}}, + {"?range=week", []int{1, 12, 30, 100}}, + {"?hours=99999", []int{1, 12, 30, 100}}, // clamped to 720h, so 800 excluded + } { + rec := doGet(s, "/api/trends"+tc.query) + if rec.Code != http.StatusOK { + t.Errorf("trends%s status = %d", tc.query, rec.Code) + continue + } + + var body struct { + TokenHistory []TokenSparklineEntry `json:"tokenHistory"` + } + if err := json.NewDecoder(rec.Body).Decode(&body); err != nil { + t.Errorf("trends%s: decode: %v", tc.query, err) + continue + } + + got := make([]int, 0, len(body.TokenHistory)) + for _, e := range body.TokenHistory { + got = append(got, int(e.Input)) + } + sort.Ints(got) + + want := append([]int(nil), tc.wantAges...) + sort.Ints(want) + + if !reflect.DeepEqual(got, want) { + t.Errorf("trends%s returned entries aged %v, want %v — "+ + "the time-window parameter is not being honoured", tc.query, got, want) + } + } + + // The five variants must not all mean the same thing, or the table above + // could be satisfied by a handler with a single fixed window. + dayRec := doGet(s, "/api/trends?range=day") + weekRec := doGet(s, "/api/trends?range=week") + if dayRec.Body.String() == weekRec.Body.String() { + t.Error("range=day and range=week returned identical bodies — " + + "handleTrends is ignoring the range parameter") + } +} + // ---------- Token access (no file present → empty entries) ---------- func TestCovG2_TokenAccess(t *testing.T) { @@ -306,14 +404,30 @@ func TestCovG2_BudgetIgnore(t *testing.T) { if rec := doGet(s, "/api/budget-ignore"); rec.Code != http.StatusOK { t.Errorf("budget-ignore get = %d", rec.Code) } - // Bool form. + // Bool form. #5388: assert the write actually took, not just that the + // handler returned 200 — a no-op handler satisfies the status alone. if rec := doPost(s, "/api/budget-ignore", map[string]any{"ignored": true}); rec.Code != http.StatusOK { t.Errorf("budget-ignore bool = %d", rec.Code) + } else if got := decodeJSON(t, rec)["ignored"]; got != true { + t.Errorf("budget-ignore bool: response ignored = %v, want true — the global bypass was not applied", got) + } + if got := decodeJSON(t, doGet(s, "/api/budget-ignore"))["ignored"]; got != true { + t.Errorf("budget-ignore: re-read ignored = %v, want true — the bool form did not persist", got) + } + // Setting it back to false must also take, or the field is simply stuck on. + if rec := doPost(s, "/api/budget-ignore", map[string]any{"ignored": false}); rec.Code != http.StatusOK { + t.Errorf("budget-ignore bool false = %d", rec.Code) + } + if got := decodeJSON(t, doGet(s, "/api/budget-ignore"))["ignored"]; got != false { + t.Errorf("budget-ignore: re-read ignored = %v, want false — the toggle is stuck on", got) } // List form. if rec := doPost(s, "/api/budget-ignore", map[string]any{"ignored": []string{"scanner"}}); rec.Code != http.StatusOK { t.Errorf("budget-ignore list = %d", rec.Code) } + if got := decodeJSON(t, doGet(s, "/api/budget-ignore"))["agents"]; !reflect.DeepEqual(got, []any{"scanner"}) { + t.Errorf("budget-ignore: re-read agents = %v, want [scanner] — the per-agent exemption list did not persist", got) + } // Bad body → 400. if rec := covG2ServeRaw(s, http.MethodPost, "/api/budget-ignore", `{bad`); rec.Code != http.StatusBadRequest { t.Errorf("budget-ignore bad body = %d, want 400", rec.Code) @@ -416,19 +530,27 @@ func TestCovG2_KnowledgeCluster(t *testing.T) { } func TestCovG2_KnowledgeToggle(t *testing.T) { - s, _ := apiServer(t) + s, deps := apiServer(t) // Bad body → 400. if rec := covG2ServeRaw(s, http.MethodPut, "/api/knowledge/enabled", `{bad`); rec.Code != http.StatusBadRequest { t.Errorf("knowledge toggle bad body = %d, want 400", rec.Code) } - // Enable then disable. + // Enable then disable. #5388: a handler that parsed the body and did + // nothing with it passed the status-only form of this test in both + // directions; assert the config field the toggle exists to write. if rec := doPut(s, "/api/knowledge/enabled", map[string]bool{"enabled": true}); rec.Code != http.StatusOK { t.Errorf("knowledge toggle enable = %d", rec.Code) } + if !deps.Config.Knowledge.Enabled { + t.Error("knowledge toggle enable returned 200 but Config.Knowledge.Enabled is false — the toggle did not take") + } if rec := doPut(s, "/api/knowledge/enabled", map[string]bool{"enabled": false}); rec.Code != http.StatusOK { t.Errorf("knowledge toggle disable = %d", rec.Code) } + if deps.Config.Knowledge.Enabled { + t.Error("knowledge toggle disable returned 200 but Config.Knowledge.Enabled is still true — the toggle is stuck on") + } } func TestCovG2_Vaults(t *testing.T) { @@ -511,19 +633,28 @@ func TestCovG2_Documents(t *testing.T) { // ---------- Bead synthesizer ---------- func TestCovG2_BeadSynth(t *testing.T) { - s, _ := apiServer(t) + s, deps := apiServer(t) if rec := doGet(s, "/api/knowledge/bead-synthesizer"); rec.Code != http.StatusOK { t.Errorf("bead-synth status = %d", rec.Code) } if rec := covG2ServeRaw(s, http.MethodPut, "/api/knowledge/bead-synthesizer/enabled", `{bad`); rec.Code != http.StatusBadRequest { t.Errorf("bead-synth bad body = %d, want 400", rec.Code) } + // #5388: assert the toggle writes the config field it exists to write. + // IsEnabled() is the accessor the rest of the codebase reads, so assert + // through it rather than the raw pointer. if rec := doPut(s, "/api/knowledge/bead-synthesizer/enabled", map[string]bool{"enabled": true}); rec.Code != http.StatusOK { t.Errorf("bead-synth enable = %d", rec.Code) } + if !deps.Config.Knowledge.BeadSynthesizer.IsEnabled() { + t.Error("bead-synth enable returned 200 but BeadSynthesizer.IsEnabled() is false — the toggle did not take") + } if rec := doPut(s, "/api/knowledge/bead-synthesizer/enabled", map[string]bool{"enabled": false}); rec.Code != http.StatusOK { t.Errorf("bead-synth disable = %d", rec.Code) } + if deps.Config.Knowledge.BeadSynthesizer.IsEnabled() { + t.Error("bead-synth disable returned 200 but BeadSynthesizer.IsEnabled() is still true — the toggle is stuck on") + } } // ---------- Git sources ---------- From b92b10cf4d7e9776b19ea7febb9602fad74ecdf2 Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Tue, 1 Sep 2026 09:47:27 -0400 Subject: [PATCH 007/190] =?UTF-8?q?=F0=9F=90=9B=20fix(contribute-ws):=20st?= =?UTF-8?q?op=20misattributing=20flaps=20to=20the=20heartbeat,=20absorb=20?= =?UTF-8?q?reconnect=20churn?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "heartbeat ping failed, closing" line that #5090 has been chasing is a LAGGING INDICATOR, not a cause. heartbeatLoop has no done channel. When the read loop's disconnect defer fires it deletes the connection and closes the socket on the read goroutine; the heartbeat goroutine learns nothing, sleeps out the rest of its 30s tick, then writes a ping to an already-closed socket and logs the failure. Because the tick is a fixed offset from REGISTRATION rather than from any peer event, that line lands ~29-30s after every "new connection" no matter what actually killed the socket. #5090's own logs show the arithmetic: every ping failure is exactly 29s after its new connection, and the disconnect precedes it (09:37:48 connect -> 09:38:00 disconnect -> 09:38:30 ping failure). The socket was dead 30s before the line that appeared to diagnose it. That fixed offset is what made the flap look like a clean 30s idle timer and sent the diagnosis toward per-direction proxy timeouts. - heartbeatLoop now returns when its connection is no longer registered, so the ping-failure line is emitted ONLY when the heartbeat write is genuinely the first thing to notice a bad socket. Also stops leaking a goroutine for up to one interval per disconnect (one per flap on a flapping session). - send() now sets a write deadline. gorilla applies none by default, so WriteJSON against a half-open socket blocks until the OS gives up on retransmission — while holding writeMu, which parks the heartbeat, the read loop's replies and the operator revoke/yank/reassign paths for that connection behind a lock nothing can break. #5151: a ~1s reconnect no longer churns three feed rows. The left/joined pair (and the "released: connection lost" preceding it) is retracted on the "joined" that closes the round trip — collapsed only once the reconnect has PROVEN the contributor returned, so a genuine departure keeps every row exactly as today, with no timer and no grace period. Flaps stay countable via absorbedReconnects and the #5107 log instrumentation is untouched. The #2356 duplicate-PR guarantee is NOT weakened: the release cooldown is still booked eagerly by the disconnect defer and is withdrawn only by the lease-bound resume (#5322), where the original owner re-entering activeIssues is the stronger guard the cooldown stood in for. No window is ever open in which the issue is both out of activeIssues and out of cooldown. Pinned by a direct test. Fixes #5151 Refs #5090 Signed-off-by: Andrew Anderson --- .../contribute_reconnect_flap_test.go | 280 ++++++++++++++++++ src/pkg/dashboard/contribute_ws.go | 218 +++++++++++++- 2 files changed, 493 insertions(+), 5 deletions(-) create mode 100644 src/pkg/dashboard/contribute_reconnect_flap_test.go 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..1f0d39610 --- /dev/null +++ b/src/pkg/dashboard/contribute_reconnect_flap_test.go @@ -0,0 +1,280 @@ +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") + } + + got := hub.RecentActivity() + if len(got) > 3 { + t.Fatalf("20 flaps should not fill the feed, got %d rows: %v", len(got), actions(got)) + } + if got[0].Action != "picked up" { + t.Fatalf("the real 'picked up' row was evicted by flap churn: %v", actions(got)) + } + if n := hub.AbsorbedReconnects(); n != 20 { + t.Fatalf("absorbed reconnect counter = %d, want 20", 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_ws.go b/src/pkg/dashboard/contribute_ws.go index ec8d65da6..0d583f15f 100644 --- a/src/pkg/dashboard/contribute_ws.go +++ b/src/pkg/dashboard/contribute_ws.go @@ -183,6 +183,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) } @@ -432,11 +450,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 @@ -954,6 +976,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, @@ -980,6 +1027,92 @@ 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 two rows: the "left", then optionally the "released: connection lost" + // that preceded it. Bounded explicitly rather than by a general scan so this can + // never chew through the feed. + for i > 0 && end-i < 2 { + 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 + } + 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() @@ -3878,6 +4011,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() @@ -3973,6 +4135,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). @@ -5504,6 +5696,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. // From cc6dc9cfe1bf8fcea6e0e9ffc0141746bd5c870c Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Tue, 1 Sep 2026 09:50:24 -0400 Subject: [PATCH 008/190] =?UTF-8?q?=F0=9F=90=9B=20Signal=20failed=20token?= =?UTF-8?q?=20re-mints=20to=20the=20relay=20and=20read=20token=5Fexpires?= =?UTF-8?q?=5Fat?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refresh itself works and is untouched: wsTokenRefreshPeriod (50m) re-mints against the 55m wsTokenTTL and re-arms each cycle, and resumeTaskToken() re-mints on reconnect, so a 4-hour task never loses push access. The two failure modes AROUND it were both silent. 1. A failed re-mint was invisible to the relay. maybeRefreshToken() logged hub-side, left the old token in place, and retried next heartbeat, telling the relay nothing — so the observable symptom was a push that started failing roughly an hour into a long task, reported to the agent as a generic auth error (the misleading-symptom class of #5343). The hub now sends token_refresh_failed from both the heartbeat and the resume path, carrying a fixed caller-supplied reason and never the mint error itself (which can quote App API responses) or any token material. Advertised as a server capability so a client can learn it without probing. Send failures are swallowed: this is a notice about a degraded credential, and failing the refresh path because the notice could not be delivered would turn a warning into an outage. The retry policy is unchanged — tokenMintedAt does not advance, so the next heartbeat still retries. 2. tokenExpiresAt was assigned in two places and compared against nothing. The relay now derives a lifetime status from it and warns, throttled, once the credential is inside a five-minute window or already past expiry — more pointedly when a failed renewal was also reported. Deliberately warn-only, with no refusal. token_expires_at is the hub's wall clock read on the relay's, so refusing a push on a few minutes of skew would abandon work on a perfectly valid credential — strictly worse than today. GitHub's answer to the real call stays the authority. Credential lifecycle is respected: the failure flag is cleared wherever the credential it describes is replaced or dropped (task_assign, a successful token_refresh, dropTaskCredential), and nothing is dropped on the task_assign decline paths, which per #5373 would destroy the credential of the task still being worked. Concurrency: sendTokenRefreshFailed goes through c.send (writeMu) and takes no other lock; both callers hold neither c.mu nor c.writeMu, as tokenRefreshDue releases c.mu before returning. Tests assert observable behaviour rather than reachability (#5388): three Go tests read the actual frame off a real socket pair and fail with "the relay was never told" against the old code, and seven relay tests fail without the change. The pre-existing mint-failure test moves off its nil-ws tripwire, since that path now legitimately writes one non-credential frame. Docs: contributor-relay.md said "a failed re-mint is not announced" and "expiry is advertised but not enforced" — both now stale. Fixes #5447 Signed-off-by: Andrew Anderson --- CHANGELOG.md | 4 + bin/contributor-relay.sh | 119 +++++++++++++ bin/contributor-relay.test.js | 165 ++++++++++++++++++ src/docs/contributor-relay.md | 41 +++-- src/pkg/dashboard/contribute_protocol.go | 11 ++ src/pkg/dashboard/contribute_ws.go | 43 +++++ .../contribute_ws_maybe_refresh_test.go | 129 +++++++++++++- 7 files changed, 495 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 128a22493..8995ea826 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ Hive did not historically maintain a complete changelog. This file starts a prag ## Unreleased +### Fixed + +- A contributor token re-mint that fails is no longer invisible to the relay, and the `token_expires_at` the hub already sends is finally read ([#5447](https://github.com/kubestellar/hive/issues/5447)). Refresh itself was working and is unchanged — the 50-minute re-mint against a 55-minute TTL still re-arms each cycle, so a 4-hour task never loses push access. What was missing was what happens when a mint *fails*: the hub logged a warning and told the relay nothing, so the first observable symptom was a push that started failing roughly an hour into a long task, surfaced to the agent as a generic authentication error. The hub now sends a `token_refresh_failed` message — a reason string, never token material — from both the heartbeat and the task-resume paths, advertised in the `auth_ok` capability set, and the relay logs it against the task it belongs to. Separately, the relay now compares `token_expires_at` against the clock on each progress tick and warns when the credential is within five minutes of expiry or already past it, more pointedly when a failed renewal was also reported. Both halves are advisory by design: nothing is revoked, no task is failed, and **no push is refused** — `token_expires_at` is the hub's wall clock read on the relay's, so refusing work on a few minutes of clock skew would be worse than the current behavior. GitHub's answer to the actual call remains the authority; these changes only ensure that when it does fail, the cause is already named in the log. A relay that ignores the new message behaves exactly as before. + ## 2026-09-01 (v4.0.1) ### Added diff --git a/bin/contributor-relay.sh b/bin/contributor-relay.sh index 552d315fd..47e87e20b 100755 --- a/bin/contributor-relay.sh +++ b/bin/contributor-relay.sh @@ -341,6 +341,80 @@ let seq = 0; let currentTask = null; let progressInterval = null; let tokenExpiresAt = null; +// tokenRefreshFailedAt records when the hub last told us a mid-task re-mint +// FAILED (a token_refresh_failed, kubestellar/hive#5447). Null means "no known +// refresh problem"; a successful token_refresh clears it, because a fresh +// credential resolves the condition. It exists so the expiry warning below can +// distinguish "the hub is quiet and our clock may simply be off" from "the hub +// told us it could not renew this credential", which is the difference between a +// guess and a diagnosis. +let tokenRefreshFailedAt = null; + +// TOKEN_EXPIRY_WARN_MS is how far ahead of expiry the relay starts warning. It +// is one full progress interval plus a margin, so a task that is about to lose +// push access says so at least one tick BEFORE the first push can fail, rather +// than reporting it afterwards. +const TOKEN_EXPIRY_WARN_MS = 5 * 60 * 1000; +// TOKEN_EXPIRY_WARN_INTERVAL_MS throttles the warning so a long task past expiry +// logs periodically instead of on every single progress tick. +const TOKEN_EXPIRY_WARN_INTERVAL_MS = 10 * 60 * 1000; +let lastTokenExpiryWarnAt = 0; + +// tokenLifetimeStatus turns the hub-supplied token_expires_at into the relay's +// own read of its credential: how long is left, whether we are inside the warning +// window, and whether the hub has reported a failed renewal. +// +// It is PURE and clock-injectable so the expiry logic can be tested without +// waiting an hour, and it deliberately reports rather than decides — see +// warnOnTokenExpiry() for why this only ever warns. +function tokenLifetimeStatus(now = Date.now()) { + if (!tokenExpiresAt) { + return { known: false, expired: false, expiring: false, remainingMs: null, refreshFailed: tokenRefreshFailedAt !== null }; + } + const remainingMs = tokenExpiresAt - now; + return { + known: true, + expired: remainingMs <= 0, + expiring: remainingMs <= TOKEN_EXPIRY_WARN_MS, + remainingMs, + refreshFailed: tokenRefreshFailedAt !== null, + }; +} + +function formatDuration(ms) { + const abs = Math.abs(ms); + const mins = Math.floor(abs / 60000); + const secs = Math.floor((abs % 60000) / 1000); + return mins > 0 ? `${mins}m${secs}s` : `${secs}s`; +} + +// warnOnTokenExpiry logs — and ONLY logs — when the task's credential is at or +// past its advertised expiry (kubestellar/hive#5447). +// +// It does NOT refuse the push, and that is deliberate. The relay's clock and the +// hub's are independent; tokenExpiresAt is the HUB's wall-clock stamp read on the +// relay's, so a machine with a few minutes of skew would refuse work on a +// perfectly valid credential. Refusing on a bad clock is strictly worse than +// today's behaviour, where the token simply works. The authority on whether a +// token is good remains GitHub's answer to the actual call; this turns the +// resulting failure from an unexplained auth error into a named, already-logged +// condition — which is the whole point of the issue. +// +// Throttled, and never touches the token itself. +function warnOnTokenExpiry(now = Date.now()) { + const status = tokenLifetimeStatus(now); + if (!status.known || !status.expiring) return null; + if (now - lastTokenExpiryWarnAt < TOKEN_EXPIRY_WARN_INTERVAL_MS) return null; + lastTokenExpiryWarnAt = now; + const cause = status.refreshFailed + ? ' — the hub reported that it could not renew this credential, so pushes may fail with a generic auth error' + : ''; + const msg = status.expired + ? `GitHub token expired ${formatDuration(status.remainingMs)} ago${cause}` + : `GitHub token expires in ${formatDuration(status.remainingMs)}${cause}`; + console.warn(msg); + return msg; +} function nextSeq() { return ++seq; } @@ -2383,6 +2457,11 @@ function relaunchCLI() { function dropTaskCredential() { try { fs.unlinkSync(GH_TOKEN_CACHE); } catch (_) {} tokenExpiresAt = null; + // The credential this failure was ABOUT is gone, so the condition dies with + // it — otherwise a stale "refresh failed" would colour the next task's + // warnings (#5447). + tokenRefreshFailedAt = null; + lastTokenExpiryWarnAt = 0; } // stopAgentForTaskExit ends the AGENT, not just the bookkeeping, when a task @@ -3011,6 +3090,13 @@ function maybeSendAutonomyNudge(tmuxLines) { function progressTick() { lastProgressTick = Date.now(); if (!currentTask) return; + + // Surface the credential's remaining lifetime BEFORE the grace-period return + // and before any of the pane judging below, so a token that is about to lapse + // is reported on its own schedule rather than only on ticks that happen to get + // as far as a progress report (#5447). Warn-only — see warnOnTokenExpiry. + warnOnTokenExpiry(); + if (Date.now() - taskAssignedAt < TASK_GRACE_PERIOD_MS) return; // #4117: re-detect the running model each tick so a mid-session model switch @@ -3435,6 +3521,9 @@ function handleMessage(data, hub) { if (msg.github_token) { injectGhToken(msg.github_token); tokenExpiresAt = msg.token_expires_at ? new Date(msg.token_expires_at).getTime() : null; + // Fresh task, fresh credential: no inherited refresh failure (#5447). + tokenRefreshFailedAt = null; + lastTokenExpiryWarnAt = 0; } // TASK_FILE is observability/debug state with no reader that needs the // credential; the live token's one legitimate on-disk home is the 0600 @@ -3468,10 +3557,37 @@ function handleMessage(data, hub) { if (msg.github_token) { injectGhToken(msg.github_token); tokenExpiresAt = msg.token_expires_at ? new Date(msg.token_expires_at).getTime() : null; + // A delivered credential resolves any earlier renewal failure, and + // re-arms the expiry warning for the new token's own window (#5447). + tokenRefreshFailedAt = null; + lastTokenExpiryWarnAt = 0; console.log('GitHub token refreshed'); } break; + // token_refresh_failed (kubestellar/hive#5447): the hub could not re-mint + // this task's credential. The token we hold is still the OLD one and stays + // installed — the hub retries on its next heartbeat — so there is nothing to + // drop and nothing to fail here. Recording it is the entire point: without + // it, the first evidence of a stale credential is a push failing about an + // hour into a long task, surfaced to the agent as a generic auth error + // (#5343's misleading-symptom class). + case 'token_refresh_failed': { + if (!currentTask || currentTaskHub() !== hub) { + console.log(`Ignoring token_refresh_failed from ${hub.url} — it does not own the active task`); + break; + } + tokenRefreshFailedAt = Date.now(); + const status = tokenLifetimeStatus(); + const remaining = status.known + ? (status.expired + ? `the current token expired ${formatDuration(status.remainingMs)} ago` + : `the current token expires in ${formatDuration(status.remainingMs)}`) + : 'the current token has no known expiry'; + console.error(`GitHub token refresh FAILED for ${taskKey(currentTask)}: ${msg.reason || 'no reason given'} — ${remaining}. Pushes may fail with a generic auth error until the hub renews it.`); + break; + } + case 'task_revoke': if (!currentTask) { console.log(`Ignoring task_revoked from ${hub.url} for ${msg.task_id} — no active task`); @@ -3699,6 +3815,9 @@ if (process.env.HIVE_RELAY_TEST_MODE === '1') { handleMessage, injectGhToken, GH_TOKEN_CACHE, + tokenLifetimeStatus, + warnOnTokenExpiry, + TOKEN_EXPIRY_WARN_MS, tmuxSendKeys, flushPendingTask, relaunchCLI, diff --git a/bin/contributor-relay.test.js b/bin/contributor-relay.test.js index 41b26edae..a0135d6a6 100644 --- a/bin/contributor-relay.test.js +++ b/bin/contributor-relay.test.js @@ -5512,6 +5512,171 @@ test('#5376 recordChromeIdleTick fires only after the full consecutive window', } finally { teardown(relay); } }); +// --------------------------------------------------------------------------- +// kubestellar/hive#5447 — a failed re-mint must be visible to the relay, and +// token_expires_at must actually be read. +// +// Both halves were plumbing that carried no effect: maybeRefreshToken() logged +// hub-side and told the relay nothing, so a stale credential first showed up as +// a push failing about an hour into a long task; and tokenExpiresAt was +// assigned in two places and never compared against anything. +// +// These assert OBSERVABLE behaviour (#5388): that a simulated mint failure +// produces relay-visible output naming the condition, and that the expiry check +// actually fires on a stale token rather than merely being reachable. +// --------------------------------------------------------------------------- + +// Drives a token_refresh carrying an expiry, so a test starts from a relay that +// genuinely holds a credential with a known lifetime. +function refreshToken(relay, expiresInMs) { + relay.handleMessage(JSON.stringify({ + type: 'token_refresh', + github_token: 'ghs_fake_test_token', + token_expires_at: new Date(Date.now() + expiresInMs).toISOString(), + })); +} + +test('#5447 a hub-reported refresh failure is logged against the task, not swallowed', () => { + const relay = loadRelay({ backend: 'claude' }); + const origError = console.error; + const errors = []; + console.error = (...args) => { errors.push(args.join(' ')); }; + try { + relay.setCliReady(true); + assignTask(relay, 't-refresh-fail'); + refreshToken(relay, 55 * 60 * 1000); + relay.handleMessage(JSON.stringify({ + type: 'token_refresh_failed', + reason: 'mint failed, will retry on the next heartbeat', + })); + // The whole point of the issue: the relay must name the CREDENTIAL as the + // problem. Before this change nothing was emitted at all — the message type + // fell through handleMessage's switch unhandled. + const named = errors.find(e => e.includes('token refresh FAILED')); + assert.ok(named, 'a failed re-mint produced no relay-visible signal'); + assert.ok(named.includes('foo/bar#421'), 'the failure was not logged against the task it belongs to'); + assert.ok(named.includes('mint failed'), 'the hub-supplied reason did not reach the log'); + } finally { + console.error = origError; + teardown(relay); + } +}); + +test('#5447 a refresh failure for a task we do not own is ignored', () => { + const relay = loadRelay({ backend: 'claude' }); + try { + relay.setCliReady(true); + // No active task: the message must not be recorded against nothing. + relay.handleMessage(JSON.stringify({ type: 'token_refresh_failed', reason: 'mint failed' })); + assert.strictEqual(relay.tokenLifetimeStatus().refreshFailed, false, + 'a refresh failure with no active task was recorded anyway'); + } finally { teardown(relay); } +}); + +test('#5447 a later successful refresh clears the failure condition', () => { + const relay = loadRelay({ backend: 'claude' }); + try { + relay.setCliReady(true); + assignTask(relay, 't-refresh-recover'); + refreshToken(relay, 55 * 60 * 1000); + relay.handleMessage(JSON.stringify({ type: 'token_refresh_failed', reason: 'mint failed' })); + assert.strictEqual(relay.tokenLifetimeStatus().refreshFailed, true, + 'the failure was not recorded in the first place'); + refreshToken(relay, 55 * 60 * 1000); + assert.strictEqual(relay.tokenLifetimeStatus().refreshFailed, false, + 'a delivered credential must resolve the earlier renewal failure'); + } finally { teardown(relay); } +}); + +test('#5447 tokenExpiresAt is actually read — a stale token warns', () => { + const relay = loadRelay({ backend: 'claude' }); + const origWarn = console.warn; + const warnings = []; + console.warn = (...args) => { warnings.push(args.join(' ')); }; + try { + relay.setCliReady(true); + assignTask(relay, 't-expiry'); + // A credential that lapsed ten minutes ago. Before this change the relay + // held this number and never compared it to anything. + refreshToken(relay, -10 * 60 * 1000); + const msg = relay.warnOnTokenExpiry(); + assert.ok(msg, 'an expired token produced no warning — token_expires_at is still unread'); + assert.ok(/expired/.test(msg), `expected an expiry warning, got: ${msg}`); + assert.ok(warnings.some(w => /expired/.test(w)), 'the expiry warning never reached the log'); + const status = relay.tokenLifetimeStatus(); + assert.strictEqual(status.expired, true); + assert.strictEqual(status.known, true); + } finally { + console.warn = origWarn; + teardown(relay); + } +}); + +test('#5447 a healthy token neither warns nor reports expiry', () => { + const relay = loadRelay({ backend: 'claude' }); + try { + relay.setCliReady(true); + assignTask(relay, 't-healthy'); + refreshToken(relay, 50 * 60 * 1000); + assert.strictEqual(relay.warnOnTokenExpiry(), null, + 'a token with 50 minutes left must not warn — that would be noise on every task'); + const status = relay.tokenLifetimeStatus(); + assert.strictEqual(status.expired, false); + assert.strictEqual(status.expiring, false); + } finally { teardown(relay); } +}); + +test('#5447 the warning fires inside the window, before the first push can fail', () => { + const relay = loadRelay({ backend: 'claude' }); + try { + relay.setCliReady(true); + assignTask(relay, 't-window'); + // Still valid, but inside the warning window: the operator should hear + // about it BEFORE the credential lapses, not afterwards. + refreshToken(relay, Math.floor(relay.TOKEN_EXPIRY_WARN_MS / 2)); + const status = relay.tokenLifetimeStatus(); + assert.strictEqual(status.expired, false, 'this token has not expired yet'); + assert.strictEqual(status.expiring, true, 'a token inside the warning window must be flagged as expiring'); + const msg = relay.warnOnTokenExpiry(); + assert.ok(msg && /expires in/.test(msg), `expected a pre-expiry warning, got: ${msg}`); + } finally { teardown(relay); } +}); + +test('#5447 the expiry warning is throttled, not emitted every tick', () => { + const relay = loadRelay({ backend: 'claude' }); + try { + relay.setCliReady(true); + assignTask(relay, 't-throttle'); + refreshToken(relay, -10 * 60 * 1000); + assert.ok(relay.warnOnTokenExpiry(), 'the first warning must fire'); + assert.strictEqual(relay.warnOnTokenExpiry(), null, + 'a second immediate warning would spam the log on every progress tick'); + } finally { teardown(relay); } +}); + +test('#5447 an expired token still does NOT refuse the work (clock skew)', () => { + const relay = loadRelay({ backend: 'claude' }); + const origWarn = console.warn; + console.warn = () => {}; + try { + relay.setCliReady(true); + assignTask(relay, 't-no-refusal'); + refreshToken(relay, -60 * 60 * 1000); + relay.handleMessage(JSON.stringify({ type: 'token_refresh_failed', reason: 'mint failed' })); + relay.progressTick(); + // Deliberate: tokenExpiresAt is the HUB's wall clock read on OURS, so a + // skewed machine would abandon work on a perfectly good credential. + // Warning is the ceiling here; failing the task is not. + assert.ok(!relay.__sent.some(m => m.type === 'task_failed'), + 'an expired-looking token must not fail the task — clock skew would destroy live work'); + assert.strictEqual(relay.getCurrentTask() ? relay.getCurrentTask().task_id : null, 't-no-refusal', + 'the task must still be held'); + } finally { + console.warn = origWarn; + teardown(relay); + } +}); + // --------------------------------------------------------------------------- let failed = 0; diff --git a/src/docs/contributor-relay.md b/src/docs/contributor-relay.md index 6180d14cd..0c5d5f131 100644 --- a/src/docs/contributor-relay.md +++ b/src/docs/contributor-relay.md @@ -468,21 +468,32 @@ Two things follow from refresh being driven by the hub's heartbeat: re-arms the cycle — without that step the resumed session's mint time would stay zero and refresh would never fire again for the life of the connection ([#2610](https://github.com/kubestellar/hive/issues/2610)). -- **A failed re-mint is not fatal and is not announced.** If the mint errors, or - the hive has no App auth to mint from, the hub logs it and leaves the relay's - existing token in place, retrying on the next heartbeat. The relay is told - nothing. So the observable failure mode is not a "token expired" message: it - is a push or `gh` call that starts returning an authentication error partway - through a long task, with the previous 55 minutes having worked normally. - -**Expiry is advertised but not enforced by the relay.** Each `token_refresh` -carries a `token_expires_at` timestamp, and the relay records it — but it never -checks it. Nothing in the relay warns as expiry approaches, refuses to start a -push against a stale token, or asks the hub for a new one. The relay finds out -that a token has died the same way it finds out about any other GitHub error: -the command fails. If you see an authentication failure on a task that has been -running for around an hour, a re-mint that quietly failed on the hub side is the -first thing to check, and the hub's log is the only place that records it. +- **A failed re-mint is not fatal, but it is announced.** If the mint errors the + hub logs it, leaves the relay's existing token in place, and retries on the + next heartbeat — and it now also sends the relay a `token_refresh_failed` + carrying a reason and no token material, so the relay logs the condition + against the task it belongs to + ([#5447](https://github.com/kubestellar/hive/issues/5447)). Both the heartbeat + and the resume path do this, and the hub advertises `token_refresh_failed` in + its `auth_ok` capability set. The message is advisory: nothing is revoked, no + task is failed, and a relay that ignores it behaves exactly as before. The + no-App-auth case is still silent — it is a deployment posture, not a failure. + +**Expiry is now read, and warned on — but never enforced.** Each `token_refresh` +carries a `token_expires_at` timestamp. The relay records it and, on each +progress tick, compares it against the clock: it warns once the credential is +within five minutes of expiry or already past it, and says so more pointedly +when the hub has separately reported a failed renewal. The warning is throttled +to once every ten minutes so a long task does not spam its log. + +It stops at warning deliberately. `token_expires_at` is the *hub's* wall clock +read on the *relay's*, so a machine with a few minutes of skew would refuse work +on a perfectly valid credential — strictly worse than today, where the token +simply works. GitHub's answer to the actual call remains the authority on +whether a token is good; the warning exists so that when the call does fail, the +cause is already named in the log rather than surfacing as a generic +authentication error. If you see an authentication failure on a task that has +been running for around an hour, look for these two lines first. **Removal.** The token is unlinked on **every** task-exit path, before the agent is interrupted, so a turn that survives the stop cannot keep pushing against an diff --git a/src/pkg/dashboard/contribute_protocol.go b/src/pkg/dashboard/contribute_protocol.go index e813b8125..99f01fa05 100644 --- a/src/pkg/dashboard/contribute_protocol.go +++ b/src/pkg/dashboard/contribute_protocol.go @@ -79,6 +79,16 @@ const ( // offer-pool suppression instead of the short idle cooldown loop. Purely // additive: a relay that never sends the field behaves exactly as before. capCompletionVerdict = "completion_verdict" + // 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. @@ -93,6 +103,7 @@ func serverCapabilities() []string { capCredentialAfterAccept, capAgentRoleClaim, capCompletionVerdict, + capTokenRefreshFailed, } } diff --git a/src/pkg/dashboard/contribute_ws.go b/src/pkg/dashboard/contribute_ws.go index ec8d65da6..9a9cbfdcf 100644 --- a/src/pkg/dashboard/contribute_ws.go +++ b/src/pkg/dashboard/contribute_ws.go @@ -3925,6 +3925,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 == "" { @@ -4053,6 +4054,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 == "" { @@ -4088,6 +4090,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 diff --git a/src/pkg/dashboard/contribute_ws_maybe_refresh_test.go b/src/pkg/dashboard/contribute_ws_maybe_refresh_test.go index 56ea15c60..c2d1c7dbf 100644 --- a/src/pkg/dashboard/contribute_ws_maybe_refresh_test.go +++ b/src/pkg/dashboard/contribute_ws_maybe_refresh_test.go @@ -88,10 +88,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 +105,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 +237,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) +} From 95ca7242626d7e39011e54ad217aba2f080e7552 Mon Sep 17 00:00:00 2001 From: Andy Anderson Date: Tue, 1 Sep 2026 09:54:07 -0400 Subject: [PATCH 009/190] =?UTF-8?q?=F0=9F=90=9B=20fix(escalation):=20machi?= =?UTF-8?q?nery-generation=20amnesty=20+=20larger=20re-engagement=20budget?= =?UTF-8?q?=20(#5471)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine console PRs sat permanently needs-human because their 3 re-engagement attempts were burned under generation-1 fix dispatch — kicks that carried no CI evidence, no branch name, no push-to-branch instruction; most 'attempts' never produced a single commit (every escalated PR had exactly one commit). Escalation is a one-way door, so when #4828 fixed the machinery, the already-escalated PRs stayed outside the loop forever. Introduce MachineryVersion (now 2): entries whose attempts were burned under an older generation get ONE fresh budget — un-escalated, counters cleared, distinct-SHA ledger restarted (without the ledger reset the next sweep would instantly re-escalate). The amnesty is one-shot per generation bump; the loop-breaker semantics are unchanged within a generation. Also raise MaxReEngagements 3→6: evidence-rich kicks make each attempt materially likelier to land, so the breaker can afford more patience before paging a human. Signed-off-by: Andrew Anderson --- src/pkg/escalation/escalation.go | 45 +++++++++++++++++++++-- src/pkg/escalation/escalation_test.go | 52 +++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/src/pkg/escalation/escalation.go b/src/pkg/escalation/escalation.go index 385bbbe30..cb232e01a 100644 --- a/src/pkg/escalation/escalation.go +++ b/src/pkg/escalation/escalation.go @@ -47,7 +47,23 @@ const RedPRStaleAfter = 10 * time.Minute // re-dispatched every tick forever. Distinct from DefaultThreshold, which // counts distinct red SHAs (real fix attempts); this counts re-nudges of an // unchanged red SHA. -const MaxReEngagements = 3 +const MaxReEngagements = 6 + +// MachineryVersion identifies the GENERATION of the fix-dispatch machinery. +// Bump it when the kick/repair pipeline changes materially enough that +// attempts burned under the previous generation are no longer predictive of +// the next attempt's success. Entries whose recorded generation is older get +// ONE fresh set of re-engagements (and are un-escalated) on their next +// TryReEngage — without this, a PR escalated under machinery that could not +// possibly have fixed it (pre-#4828 kicks carried no CI evidence, no branch +// name, no push-to-branch instruction, and most "attempts" never produced a +// single commit) stays human-parked forever even after the machinery is +// repaired. Observed on kubestellar/console 2026-09-01: nine split PRs +// escalated under generation-1 no-op attempts, permanently outside the loop. +// +// Generation 2: evidence-rich FIX-BEFORE-NEW kicks (#4828) + per-agent +// attribution + AGENTS.md repair contracts. +const MachineryVersion = 2 // Entry is the persisted per-PR attempt record. type Entry struct { @@ -76,6 +92,10 @@ type Entry struct { // PR goes green. The re-engagement cap (MaxReEngagements) reads this so a // permanently-red, never-moving PR is not nudged forever. ReEngagements int `json:"re_engagements,omitempty"` + // Machinery is the MachineryVersion under which this entry's attempts + // were burned. Older-generation entries are granted amnesty (see + // MachineryVersion). + Machinery int `json:"machinery,omitempty"` } // Store is the on-PVC attempt ledger. All methods are safe for concurrent use. @@ -149,9 +169,20 @@ func (s *Store) Sweep(obs []Observation, threshold int) map[string]Result { } e := s.entries[key] if e == nil { - e = &Entry{} + e = &Entry{Machinery: MachineryVersion} s.entries[key] = e } + // Machinery amnesty (see MachineryVersion): attempts and escalations + // burned under an older fix-dispatch generation are wiped once, and + // the distinct-SHA ledger restarts, so the CURRENT machinery gets its + // own budget before a human is paged again. Without the RedSHAs reset + // the very next sweep would re-escalate on the old ledger. + if e.Machinery < MachineryVersion { + e.Machinery = MachineryVersion + e.ReEngagements = 0 + e.Escalated = false + e.RedSHAs = nil + } if o.HeadSHA != "" && !containsSHA(e.RedSHAs, o.HeadSHA) { e.RedSHAs = append(e.RedSHAs, o.HeadSHA) if len(e.RedSHAs) > maxTrackedSHAs { @@ -333,6 +364,16 @@ func (s *Store) TryReEngage(repo string, number int, headSHA string) bool { e.FirstRedAt = s.now() e.ReEngagements = 0 } + // Machinery amnesty: attempts burned under an older fix-dispatch + // generation don't count against the current one. Grant one fresh set + // and pull the PR back out of the escalated (needs-human) state so the + // current machinery gets its own chance before a human is paged again. + if e.Machinery < MachineryVersion { + e.Machinery = MachineryVersion + e.ReEngagements = 0 + e.Escalated = false + e.RedSHAs = nil + } if e.ReEngagements >= MaxReEngagements { return false } diff --git a/src/pkg/escalation/escalation_test.go b/src/pkg/escalation/escalation_test.go index e69913aec..872d7d4c4 100644 --- a/src/pkg/escalation/escalation_test.go +++ b/src/pkg/escalation/escalation_test.go @@ -228,3 +228,55 @@ func TestSweep_EscalatesWhenReEngagementBudgetExhaustedOnUnchangedSHA(t *testing t.Fatalf("new SHA resets the budget; must not escalate yet: got %+v", got) } } + +// Machinery amnesty: entries escalated under an older fix-dispatch generation +// (pre-#4828 kicks carried no CI evidence and most attempts produced no +// commit) get ONE fresh budget under the current generation — un-escalated, +// counters cleared, distinct-SHA ledger restarted — instead of staying +// human-parked forever. Entries already at the current generation keep their +// state untouched. +func TestSweep_MachineryAmnestyReleasesOldGenerationEscalations(t *testing.T) { + path := filepath.Join(t.TempDir(), "streaks.json") + s := Load(path) + + // Simulate a generation-1 entry: escalated, budget exhausted, full ledger. + key := Key("org/repo", 9) + s.mu.Lock() + s.entries[key] = &Entry{ + RedSHAs: []string{"a", "b", "c"}, + Escalated: true, + CurRedSHA: "c", + ReEngagements: MaxReEngagements, + Machinery: 1, + } + s.mu.Unlock() + + // Next sweep under generation 2: amnesty fires — not escalated, ledger + // restarted at the observed SHA only, and it must NOT immediately + // re-escalate off the old ledger. + r := s.Sweep([]Observation{obs("org/repo", 9, "c", true)}, 3) + got := r[key] + if got.NewlyEscala { + t.Fatalf("amnestied entry must not re-escalate on the old ledger: %+v", got) + } + if got.Attempts != 1 { + t.Fatalf("ledger must restart: got %+v", got) + } + s.mu.Lock() + stillEscalated := s.entries[key].Escalated + s.mu.Unlock() + if stillEscalated { + t.Fatal("entry must be un-escalated after amnesty") + } + // Re-engagement budget is fresh. + if !s.TryReEngage("org/repo", 9, "c") { + t.Fatal("amnestied entry must have a fresh re-engagement budget") + } + // The amnesty fires ONCE: exhausting the fresh budget escalates again. + for i := 0; i < MaxReEngagements; i++ { + s.TryReEngage("org/repo", 9, "c") + } + if s.TryReEngage("org/repo", 9, "c") { + t.Fatal("cap must hold at the current generation — amnesty is one-shot") + } +} From e1f3363601532c8340ff8970fc5f67528fdca0ce Mon Sep 17 00:00:00 2001 From: Andy Anderson Date: Tue, 1 Sep 2026 10:02:24 -0400 Subject: [PATCH 010/190] =?UTF-8?q?=F0=9F=93=9D=20Correct=20orphaned-pod?= =?UTF-8?q?=20reaper=20root-cause=20note=20with=20measured=20evidence=20(#?= =?UTF-8?q?5475)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reaper's header documented the orphan signature as "a node disappearing without draining", with spot reclaim / autoscaler scale-down / node lifecycle named as the likely triggers. That was inferred from absent spec fields rather than measured, and measurement against the affected cluster disproves it: - No node was lost in the orphan window. Every node has been continuously Running with no Machine ever deleted, and the newest node predates the oldest orphan by weeks. No autoscaler and no spot/preemptible capacity is in play. - Orphan onsets arrive in tight batches, several within the same second, repeatedly, across ten separate days. Node loss would tie a batch to a single node; these batches span namespaces spread across many nodes. - The largest batches land inside the auto-upgrade window, and per-namespace ReplicaSet history shows one new ReplicaSet per namespace per day at that same moment. The real mechanism is ordinary rolling redeploy: the daily auto-upgrade re-applies each hosted spoke Deployment, the RollingUpdate replaces the pod, and occasionally the outgoing pod's delete is never confirmed, producing exactly this signature. This explains why orphans recur on a healthy static cluster and why they concentrate in the most frequently redeployed namespaces. Doc-only. The reaper predicate is unchanged and remains unloosened. Refs #5328 Signed-off-by: Andrew Anderson --- src/pkg/hub/orphaned_pod_reaper.go | 51 ++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/src/pkg/hub/orphaned_pod_reaper.go b/src/pkg/hub/orphaned_pod_reaper.go index c7e9351dd..aa234532a 100644 --- a/src/pkg/hub/orphaned_pod_reaper.go +++ b/src/pkg/hub/orphaned_pod_reaper.go @@ -20,23 +20,42 @@ import ( // - the namespace still had exactly 1 healthy Running pod, so the live spoke // was unaffected in every case. // -// That is the fingerprint of a node disappearing without draining. The API -// server records the deletion and waits for the kubelet to confirm it; the -// kubelet is gone, so confirmation never arrives, and because no finalizer and -// no controller owns the remaining work the object persists forever. The -// distribution — 15 namespaces with 1-4 orphans each rather than one bad -// namespace with 27 — argues for a recurring cluster-level event (spot -// reclaim, autoscaler scale-down, or a node lifecycle event) rather than a -// single misbehaving spoke. +// The ORIGINAL reading of that signature was "a node disappeared without +// draining": the API server records the deletion and waits for the kubelet to +// confirm it, the kubelet is gone, so confirmation never arrives, and with no +// finalizer and no owning controller the object persists forever. That reading +// was inferred from absent spec fields rather than measured, and MEASUREMENT +// HAS SINCE DISPROVEN IT. Recorded here because the wrong cause is the kind of +// thing that gets re-derived from the same signature by the next reader: // -// WHY A REAPER IS THE RIGHT FIX EVEN THOUGH IT TREATS THE SYMPTOM. Finding and -// fixing whatever removes nodes ungracefully is issue #5328 item 1 and is NOT -// this lane. But ungraceful node loss can always recur — it is a property of -// the infrastructure, not a bug that stays fixed — so a reaper remains useful -// after the root cause lands. What makes the accumulation expensive is not any -// single orphan but that NOTHING sweeps them: the condition is self- -// perpetuating, so the count only ever grows between manual interventions, and -// it grew for three weeks with no alert. +// - No node was lost in the orphan window. Every node on the affected +// cluster has been continuously Running with no Machine ever deleted, and +// the newest node predates the oldest orphan by weeks. There is no +// autoscaler and no spot/preemptible capacity in play. +// - Orphan onsets arrive in tight BATCHES — several within the same second, +// repeatedly, across ten separate days. Node loss would tie a batch to one +// node; these batches span namespaces spread over many nodes. +// - The largest batches land inside the hive AUTO-UPGRADE window (see +// autoUpgradeDailyHour in upgrade_schedule.go), and per-namespace +// ReplicaSet history shows one new ReplicaSet per namespace per day at +// that same moment. +// +// The actual mechanism is ORDINARY ROLLING REDEPLOY, not node loss. The daily +// auto-upgrade re-applies each hosted spoke Deployment; the RollingUpdate +// replaces the pod; and occasionally the outgoing pod's delete is never +// confirmed, leaving exactly this signature. That is why the orphans recur on +// a healthy, static cluster, and why they concentrate in the namespaces that +// get redeployed most often. +// +// WHY A REAPER REMAINS THE RIGHT FIX. The trigger is a routine, deliberate, +// recurring operation that the fleet depends on — not an infrastructure fault +// to be engineered away. Slowing or suppressing redeploys to avoid a rare +// unconfirmed delete would trade a cosmetic accounting problem for a real loss +// of upgrade cadence. What makes the accumulation expensive is not any single +// orphan but that NOTHING sweeps them: the condition is self-perpetuating, so +// the count only ever grows between manual interventions, and it grew for +// three weeks with no alert. A sweep is the proportionate response, and it +// stays correct regardless of which delete goes unconfirmed or why. // // IMPACT. Orphans hold their scheduler slot until forcibly removed, and // anything counting pods per namespace sees phantom replicas — a namespace From 3890c4d8eebeca04d4e5688a38f1276f72c8fffe Mon Sep 17 00:00:00 2001 From: Douglas Baggett Date: Mon, 31 Aug 2026 20:43:16 -0400 Subject: [PATCH 011/190] =?UTF-8?q?=E2=9C=A8=20test:=20live=20backend=20sm?= =?UTF-8?q?oke=20for=20the=20contributor=20CLI=20integration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hive drives vendor coding-agent CLIs (claude, codex, ...) through the contributor relay, and that seam drifts as vendors ship updates. Every existing test pins the seam against captured fixtures, so vendor changes ship green here and break in production. This adds the live complement: bin/test_backend_smoke.sh drives the REAL relay against a fake ws hub and, where credentials exist, the real CLI on a one-line task, asserting the machine-checkable contract — the task wire shape, the HIVE_VERDICT sentinel, and completion_signal=verdict rather than the chrome_idle fallback. Its first full run caught three live bugs, fixed here: - detectHiveVerdict only tolerated codex's bullet (•, U+2022). Claude Code renders assistant lines with ● (U+25CF), so every interactive claude completion silently degraded to the chrome_idle fallback the sentinel exists to replace. - headless codex hung until the task timeout: codex exec blocks on stdin-EOF even with the prompt in argv, and execFile's piped stdin was never closed. runHeadlessTask now closes the child's stdin. - codex exec refuses a non-git cwd, and the task workspace root is exactly that; the headless argv now passes --skip-git-repo-check. Wiring: - .github/workflows/backend-smoke.yml: scheduled every 6h at :45 (the :00 and :30 slots are taken), matrix backend={claude,codex} x lane={latest,pinned}. latest installs the vendor's current CLI on the runner (drift early warning); pinned runs inside ghcr.io/kubestellar/hive-contributor:latest (the versions contributors actually get). Per-arm key scoping; deduplicated per-lane issues on scheduled failures only. First and only workflow holding model credentials — deliberately its own file, outside the arm64 lane's no-secrets contract. Cheapest model tiers only (haiku / gpt-5.4-mini). - v2-ci runs the keyless subset (drift checks + stub wire-contract scenarios; live scenarios skip), so backend-list and Dockerfile-pin drift are merge-gating with zero credentials. - just backend-smoke runs it locally; HIVE_TEST_REQUIRE_BACKEND_SMOKE=1 reproduces the scheduled lane's skips-are-failures inversion. - src/Dockerfile.contributor's inline codex pin becomes ARG CODEX_VERSION, which the suite asserts equal to src/Dockerfile's — they had no guard and CAN drift (found by the suite's first keyless run). Verified: 33/33 with live claude + codex arms locally; relay suite 256/256; bin/test_bin_suites_wired.sh and check-release-lines.sh pass. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Jo76R9XXJ2iq6YcmVUiX2u Signed-off-by: Douglas Baggett --- .github/workflows/backend-smoke.yml | 223 ++++++++++ .github/workflows/v2-ci.yml | 9 + Justfile | 10 + bin/contributor-relay.sh | 24 +- bin/contributor-relay.test.js | 13 +- bin/test_backend_smoke.sh | 619 ++++++++++++++++++++++++++++ src/Dockerfile.contributor | 6 +- 7 files changed, 897 insertions(+), 7 deletions(-) create mode 100644 .github/workflows/backend-smoke.yml create mode 100644 bin/test_backend_smoke.sh diff --git a/.github/workflows/backend-smoke.yml b/.github/workflows/backend-smoke.yml new file mode 100644 index 000000000..3f85c4df7 --- /dev/null +++ b/.github/workflows/backend-smoke.yml @@ -0,0 +1,223 @@ +# Backend smoke — scheduled live canary for the contributor CLI backends. +# +# The vendor coding-agent CLIs hive drives (claude, codex) are an integration +# surface hive does not control: vendors ship updates on their own schedule, +# and what breaks is the seam — completion detection (#5376), CODEX_HOME +# handling (#5335), readiness regexes that matched nothing on real codex +# output. Every in-repo test pins that seam against captured fixtures, so a +# vendor change ships green and fails in production. This workflow runs +# bin/test_backend_smoke.sh — the real relay against a fake hub, driving the +# REAL backend CLI on a one-line task — on a schedule, and files a deduplicated +# issue when a backend integration breaks. +# +# Two lanes, deliberately, with distinct issue titles so their meanings never +# blur: +# latest — installs the vendor's current CLI release on the runner. A red +# here means vendor drift is incoming: the next pin bump will break, +# but shipped images still work. +# pinned — runs the suite inside ghcr.io/kubestellar/hive-contributor:latest, +# against the CLI versions hive actually ships. A red here means +# contributors are broken TODAY. +# +# This is the only workflow in the repo holding model credentials +# (ANTHROPIC_API_KEY / OPENAI_API_KEY repo secrets), which is exactly why it is +# its own file: the podman arm64 lane's documented contract is "no secrets", +# and nothing here may be folded into it. Keys are scoped per matrix arm — +# claude arms never see the OpenAI key and vice versa. +# +# Cost control: 4 runs/day × 2 backends × 2 lanes × 2 single-turn calls on the +# cheapest model tier (haiku / mini). The suite's live task is one line with no +# file access. NOT merge-gating — the keyless subset of the same suite runs in +# v2-ci instead. +name: Backend smoke + +on: + schedule: + # Every 6h at :45 — the :00 (coverage-hourly) and :30 (v2-tests) hourly + # slots are taken, and the offsets are deliberate repo-wide. + - cron: '45 2,8,14,20 * * *' + workflow_dispatch: + inputs: + backends: + description: 'Space-separated backends to smoke' + default: 'claude codex' + type: string + lane: + description: 'Which lane(s) to run' + type: choice + options: [both, latest, pinned] + default: 'both' + +permissions: + contents: read + +concurrency: + group: backend-smoke + cancel-in-progress: false + +jobs: + smoke: + # Forks have no model credentials; a scheduled run there would only + # produce a permanently red workflow. + if: >- + github.repository == 'kubestellar/hive' && + (github.event_name != 'workflow_dispatch' || + inputs.lane == 'both' || inputs.lane == matrix.lane) + runs-on: ubuntu-latest + timeout-minutes: 25 + strategy: + fail-fast: false + matrix: + backend: [claude, codex] + lane: [latest, pinned] + env: + # Per-arm key scoping: each backend arm gets only its own vendor's key. + ANTHROPIC_API_KEY: ${{ matrix.backend == 'claude' && secrets.ANTHROPIC_API_KEY || '' }} + OPENAI_API_KEY: ${{ matrix.backend == 'codex' && secrets.OPENAI_API_KEY || '' }} + SMOKE_BACKENDS: ${{ github.event_name == 'workflow_dispatch' && inputs.backends || 'claude codex' }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Restrict to this arm's backend + id: scope + run: | + # The matrix already splits by backend; intersect with the dispatch + # input so `backends: claude` skips the codex arms cleanly. + case " ${SMOKE_BACKENDS} " in + *" ${{ matrix.backend }} "*) echo "run=true" >> "$GITHUB_OUTPUT" ;; + *) echo "run=false" >> "$GITHUB_OUTPUT" ;; + esac + + # ── latest lane: current vendor releases on the bare runner ──────────── + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + if: steps.scope.outputs.run == 'true' && matrix.lane == 'latest' + with: + node-version: '22' + + - name: Install tmux and the latest ${{ matrix.backend }} CLI + if: steps.scope.outputs.run == 'true' && matrix.lane == 'latest' + run: | + sudo apt-get update -qq && sudo apt-get install -y -qq tmux + case "${{ matrix.backend }}" in + claude) npm install -g @anthropic-ai/claude-code@latest ;; + codex) npm install -g @openai/codex@latest ;; + esac + + - name: Backend smoke (latest ${{ matrix.backend }}) + if: steps.scope.outputs.run == 'true' && matrix.lane == 'latest' + run: | + HIVE_TEST_REQUIRE_BACKEND_SMOKE=1 \ + HIVE_SMOKE_BACKENDS='${{ matrix.backend }}' \ + bash bin/test_backend_smoke.sh |& tee smoke.log + + # ── pinned lane: the CLI versions hive ships, inside the image ───────── + - name: Pull the contributor image + if: steps.scope.outputs.run == 'true' && matrix.lane == 'pinned' + run: podman pull ghcr.io/kubestellar/hive-contributor:latest + + - name: Backend smoke (pinned ${{ matrix.backend }}, in-container) + if: steps.scope.outputs.run == 'true' && matrix.lane == 'pinned' + run: | + # The CHECKOUT's suite and relay run against the IMAGE's pinned CLI + # binaries. Secrets are forwarded by name only — never in argv. + podman run --rm --entrypoint /bin/bash \ + -e HIVE_TEST_REQUIRE_BACKEND_SMOKE=1 \ + -e HIVE_SMOKE_BACKENDS='${{ matrix.backend }}' \ + -e ANTHROPIC_API_KEY \ + -e OPENAI_API_KEY \ + -v "${GITHUB_WORKSPACE}:/opt/hive-src:ro,Z" \ + ghcr.io/kubestellar/hive-contributor:latest \ + -lc 'bash /opt/hive-src/bin/test_backend_smoke.sh' |& tee smoke.log + + - name: Upload evidence + if: always() && steps.scope.outputs.run == 'true' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: backend-smoke-${{ matrix.backend }}-${{ matrix.lane }} + path: smoke.log + if-no-files-found: ignore + + create-issue-on-failure: + # Schedule-gated, like coverage-hourly and v2-tests: a red triage dispatch + # must never file (the unscoped version of this gate gave issue #2191 a + # 41-comment pileup). Dedupe is per LANE — "vendor drift incoming" and + # "contributors broken today" are different problems with different + # urgency, and must never collapse into one issue thread. + if: failure() && github.event_name == 'schedule' + runs-on: ubuntu-latest + needs: [smoke] + permissions: + contents: read + issues: write + actions: read + steps: + - name: File or update per-lane issues + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const runUrl = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; + const sha = context.sha.substring(0, 7); + + // Which matrix arms actually failed — job names look like + // "smoke (claude, latest)". + const jobs = await github.paginate(github.rest.actions.listJobsForWorkflowRun, { + owner: context.repo.owner, + repo: context.repo.repo, + run_id: context.runId, + per_page: 100, + }); + const failed = jobs + .filter(j => j.name.startsWith('smoke (') && j.conclusion === 'failure') + .map(j => { + const m = j.name.match(/^smoke \(([^,]+), ([^)]+)\)$/); + return m ? { backend: m[1], lane: m[2] } : null; + }) + .filter(Boolean); + if (failed.length === 0) return; + + const lanes = [...new Set(failed.map(f => f.lane))]; + const existing = await github.rest.issues.listForRepo({ + owner: context.repo.owner, + repo: context.repo.repo, + state: 'open', + labels: 'backend-smoke', + per_page: 20, + }); + + for (const lane of lanes) { + const backends = failed.filter(f => f.lane === lane).map(f => f.backend); + const prefix = `[Backend smoke: ${lane}]`; + const meaning = lane === 'pinned' + ? 'The CLI versions hive SHIPS in the contributor image failed the smoke — contributors are likely broken right now.' + : "The vendor's LATEST CLI release failed the smoke against hive's relay — shipped images still work, but the next pin bump will break. Fix the integration before bumping."; + const dup = existing.data.find(i => i.title.startsWith(prefix)); + if (dup) { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: dup.number, + body: `Another scheduled failure (${backends.join(', ')}) on \`${sha}\`: ${runUrl}`, + }); + continue; + } + await github.rest.issues.create({ + owner: context.repo.owner, + repo: context.repo.repo, + title: `${prefix} ${backends.join(', ')} failure on ${sha}`, + body: [ + '## Backend smoke failure', + '', + `- **Lane:** ${lane}`, + `- **Backend(s):** ${backends.join(', ')}`, + `- **Commit:** \`${context.sha}\``, + `- **Run:** ${runUrl}`, + '', + meaning, + '', + 'The run log carries the suite\'s evidence block (relay log tail, fake-hub', + 'wire messages, tmux pane capture) for each failed scenario, and the', + '`backend-smoke--` artifact holds the full suite output.', + ].join('\n'), + labels: ['backend-smoke', 'ci-failure', 'kind/bug'], + }); + } diff --git a/.github/workflows/v2-ci.yml b/.github/workflows/v2-ci.yml index b0b2a8933..0c3ccc975 100644 --- a/.github/workflows/v2-ci.yml +++ b/.github/workflows/v2-ci.yml @@ -443,6 +443,15 @@ jobs: node --check "${RUNNER_TEMP}/contributor-relay.js" node bin/contributor-relay.test.js + # Keyless subset of the backend smoke: backend-list/Dockerfile-pin drift + # checks plus the stub wire-contract scenarios (real relay, fake hub, a + # stub CLI binary — no credentials, no network beyond localhost). The + # live scenarios skip here by design; they run with real vendor CLIs on + # a schedule in backend-smoke.yml, which is deliberately NOT merge-gating. + - name: Backend smoke (keyless subset) + working-directory: . + run: bash bin/test_backend_smoke.sh + # `just contribute-k8s` now emits a runnable contributor workload (#2549): # a headless Deployment (#2660) with a status-file probe. This runs the # real recipe and asserts the workload it produces, so a regression (a diff --git a/Justfile b/Justfile index ac4a98f43..27ee42818 100644 --- a/Justfile +++ b/Justfile @@ -227,6 +227,16 @@ contribute-check backend="claude": (contribute-check-backend backend) @echo "" @echo "✓ Machine looks ready for 'just contribute-setup {{backend}}'." +# End-to-end smoke of the contributor backend integration: the real relay +# against a fake hub, and — where a CLI + credential exist locally — the real +# backend on a one-line task. Keyless machines still run the drift checks and +# the stub wire-contract scenarios; live scenarios skip cleanly. The scheduled +# lane (.github/workflows/backend-smoke.yml) runs the same suite with skips +# escalated to failures. +# Usage: just backend-smoke (or: just backend-smoke claude) +backend-smoke backends="claude codex": + HIVE_SMOKE_BACKENDS="{{backends}}" bash bin/test_backend_smoke.sh + # One-time setup: register with hub + authenticate GitHub + authenticate CLI # Ordering note (#2543): the backend-readiness preflight runs FIRST, before # any credential is written to disk or a contributor slot is registered — diff --git a/bin/contributor-relay.sh b/bin/contributor-relay.sh index 47e87e20b..3cdc52c26 100755 --- a/bin/contributor-relay.sh +++ b/bin/contributor-relay.sh @@ -998,7 +998,11 @@ const HEADLESS_BACKENDS = { // copilot -p "" — non-interactive programmatic mode. copilot: { flag: '-p' }, // codex exec "" — Codex's non-interactive execution sub-command. - codex: { flag: 'exec' }, + // --skip-git-repo-check: exec refuses to run at all in a cwd that is not a + // git repository ("Not inside a trusted directory..."), and the task + // workspace root is exactly that — the agent clones INTO it as its first + // act. Verified live against codex 0.146.0 via bin/test_backend_smoke.sh. + codex: { flag: ['exec', '--skip-git-repo-check'] }, // goose run --no-session -t "" — goose's one-shot sub-command. The // bare `goose` binary drives the interactive TUI, but `goose run` is a // documented non-interactive entry point (#2828): `-t` takes the prompt as @@ -1188,6 +1192,13 @@ function runHeadlessTask(task) { send({ type: 'ready', seq: nextSeq() }); }); }); + // codex exec prints "Reading additional input from stdin..." and then blocks + // on stdin-EOF even with the prompt already passed as an argv element; with + // execFile's default piped stdio nothing ever closes that pipe, so a + // headless codex task produced zero output and hung until the timeout + // killed it (found live by bin/test_backend_smoke.sh). Close stdin for + // every backend — a one-shot child has no interactive input coming. + if (headlessChild && headlessChild.stdin) headlessChild.stdin.end(); } // A tmux pane can be left in bash's PS2 continuation state ("> ") when task @@ -1785,15 +1796,20 @@ function detectHiveVerdict(lines, wanted) { // Anchored at line start: the task PROMPT quotes the marker mid-sentence // ("...the exact form 'HIVE_VERDICT: ...'"), and an anchored match keeps // that instruction echo from reading as the agent's own verdict. Codex - // renders its completed assistant messages with a leading bullet, which is - // presentation chrome rather than part of the verdict. + // renders its completed assistant messages with a leading bullet (•, + // U+2022) and Claude Code with a filled circle (●, U+25CF) — presentation + // chrome rather than part of the verdict. The claude glyph was missing + // until bin/test_backend_smoke.sh drove a REAL claude pane through the + // relay: the agent printed the sentinel, this regex missed it, and every + // interactive claude completion silently degraded to the chrome_idle + // fallback the sentinel exists to replace. // // The verdict token is an alternation of exactly the wanted tokens with a \b // after it, so "no_work_neededX" and "completely rewrote the parser" are both // non-matches — a prose line that merely STARTS with a verdict word must not // become a verdict. const alt = wanted.map(w => w.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')).join('|'); - const VERDICT_RE = new RegExp(`^\\s*(?:•\\s*)?HIVE_VERDICT:\\s*(${alt})\\b[\\s:—–-]*(.*)$`, 'i'); + const VERDICT_RE = new RegExp(`^\\s*(?:[•●]\\s*)?HIVE_VERDICT:\\s*(${alt})\\b[\\s:—–-]*(.*)$`, 'i'); // Scan newest-first so the agent's final conclusion wins over anything it // merely quoted or considered earlier in the transcript. for (let i = lines.length - 1; i >= 0; i--) { diff --git a/bin/contributor-relay.test.js b/bin/contributor-relay.test.js index a0135d6a6..7ee91486a 100644 --- a/bin/contributor-relay.test.js +++ b/bin/contributor-relay.test.js @@ -2262,7 +2262,9 @@ test('buildHeadlessArgv maps each supported backend to its one-shot invocation', { backend: 'claude', tail: ['-p', PROMPT] }, { backend: 'litellm', tail: ['-p', PROMPT] }, { backend: 'copilot', tail: ['-p', PROMPT] }, - { backend: 'codex', tail: ['exec', PROMPT] }, + // --skip-git-repo-check: codex exec refuses a non-git cwd outright, and + // the task workspace root is not a repo until the agent clones into it. + { backend: 'codex', tail: ['exec', '--skip-git-repo-check', PROMPT] }, // goose needs its `run` sub-command AND -t (whose VALUE is the prompt) — // two leading tokens, unlike every other entry (#2828). { backend: 'goose', tail: ['run', '--no-session', '-t', PROMPT] }, @@ -2349,7 +2351,7 @@ test('codex headless transports model and reasoning effort without affecting oth assert.ok(a.args.includes('gpt-5.6-luna'), `codex must receive the configured model: ${JSON.stringify(a.args)}`); assert.ok(a.args.includes('-c'), `codex must receive a config override: ${JSON.stringify(a.args)}`); assert.ok(a.args.includes('model_reasoning_effort="low"'), `codex must receive the configured effort: ${JSON.stringify(a.args)}`); - assert.deepStrictEqual(a.args.slice(-2), ['exec', 'review this'], + assert.deepStrictEqual(a.args.slice(-3), ['exec', '--skip-git-repo-check', 'review this'], 'codex one-shot mode and prompt must remain at the tail'); } finally { teardown(relay); } @@ -3648,6 +3650,13 @@ test('#4267 detectNoWorkVerdict extracts the verdict and reason', () => { // Codex bullet chrome and indentation are presentation, not content. const b = relay.detectNoWorkVerdict([' • HIVE_VERDICT: no_work_needed - gated on maintainer decision']); assert.strictEqual(b.reason, 'gated on maintainer decision'); + // Claude Code renders assistant lines with ● (U+25CF), not codex's • + // (U+2022). The glyph was missing from the scanner until a live claude + // pane driven by bin/test_backend_smoke.sh showed the sentinel being + // printed and missed — every interactive claude completion degraded to + // the chrome_idle fallback. + const c = relay.detectNoWorkVerdict(['● HIVE_VERDICT: no_work_needed — backend smoke']); + assert.deepStrictEqual(c, { verdict: 'no_work_needed', reason: 'backend smoke' }); // Case-insensitive, empty reason allowed. assert.strictEqual(relay.detectNoWorkVerdict(['hive_verdict: NO_WORK_NEEDED']).verdict, 'no_work_needed'); } finally { teardown(relay); } diff --git a/bin/test_backend_smoke.sh b/bin/test_backend_smoke.sh new file mode 100644 index 000000000..6bb0453c2 --- /dev/null +++ b/bin/test_backend_smoke.sh @@ -0,0 +1,619 @@ +#!/usr/bin/env bash +# test_backend_smoke.sh — end-to-end smoke for the contributor agent backends. +# +# The vendor coding-agent CLIs hive drives (claude, codex, …) are an +# integration surface hive does not control: vendors ship CLI updates on their +# own schedule, and what breaks is the seam — completion detection (#5376), +# CODEX_HOME handling (#5335), readiness regexes that matched nothing on real +# output (see the codex arm of getCLIState in bin/contributor-relay.sh). Every +# existing test pins that seam against captured fixtures, so a vendor change +# ships green here and fails in production. This suite is the live complement: +# it drives the REAL relay against a fake hub and, where credentials exist, +# the REAL backend CLI on a one-line task, and asserts the machine-checkable +# contract — the task_complete/task_failed wire shape, the HIVE_VERDICT +# sentinel, and completion_signal=verdict rather than the chrome_idle fallback. +# +# Sections: +# A static drift checks — keyless, deterministic, always run: +# A1 HEADLESS_BACKENDS (relay) vs KNOWN_BACKENDS (backends.conf), a pair +# kept in sync by comment only before this test existed; +# A2/A3 codex + claude version pins agree across src/Dockerfile and +# src/Dockerfile.contributor. +# S stub wire-contract scenarios — keyless, deterministic: a stub backend +# binary on PATH drives the full relay↔hub loop, locking the wire shape +# the live scenarios (and the hub) rely on, with zero API spend. +# B live per-backend scenarios (needs the CLI + a credential; skips +# otherwise — fatally under HIVE_TEST_REQUIRE_BACKEND_SMOKE=1): +# B0 detect_cli health probe (contributor-agent.sh's own seam); +# B1 headless end-to-end: real relay, real CLI one-shot, fake hub; +# B2 interactive end-to-end: real CLI in tmux, the relay scraping the +# pane — the exact surface the thirteen chrome issues lived on. +# +# Env knobs: +# HIVE_SMOKE_BACKENDS space-separated, default "claude codex" +# HIVE_SMOKE_MODEL_CLAUDE default claude-haiku-4-5 (cheapest tier — +# the plumbing is under test, not the model) +# HIVE_SMOKE_MODEL_CODEX default gpt-5.4-mini +# HIVE_TEST_REQUIRE_BACKEND_SMOKE 1 = skips become failures (the CI lane +# inversion, same shape as +# HIVE_TEST_REQUIRE_BEHAVIOURAL in +# src/deploy/test_entrypoint_*.sh) +# +# Run: bash bin/test_backend_smoke.sh (keyless: A + S run, B skips) +# ANTHROPIC_API_KEY=... bash bin/test_backend_smoke.sh (full claude arm) +set -uo pipefail + +PASS=0 +FAIL=0 + +pass() { echo " PASS: $1"; PASS=$((PASS + 1)); } +fail() { + echo " FAIL: $1" + [ $# -gt 1 ] && [ -n "${2:-}" ] && echo " $2" + FAIL=$((FAIL + 1)) +} +check() { + local label="$1" want="$2" got="$3" + if [ "$want" = "$got" ]; then pass "$label"; else fail "$label" "want: '$want' got: '$got'"; fi +} +contains() { + local label="$1" haystack="$2" needle="$3" + if printf '%s' "$haystack" | grep -qF -- "$needle"; then pass "$label" + else fail "$label" "missing: '$needle'"; fi +} + +REQUIRE="${HIVE_TEST_REQUIRE_BACKEND_SMOKE:-0}" +# skip(): green-but-loud on a laptop, a real failure in the scheduled lane. +# Without the inversion this suite is the guard-that-cannot-fail: a runner +# missing every credential would skip every live scenario and stay green, +# which is exactly the state the scheduled workflow exists to rule out. +skip() { + if [ "$REQUIRE" = "1" ]; then + fail "SKIP escalated (HIVE_TEST_REQUIRE_BACKEND_SMOKE=1): $1" + else + echo " SKIP: $1" + fi +} + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +RELAY="$ROOT/bin/contributor-relay.sh" +REAL_HOME="$HOME" + +SMOKE_BACKENDS="${HIVE_SMOKE_BACKENDS:-claude codex}" +MODEL_CLAUDE="${HIVE_SMOKE_MODEL_CLAUDE:-claude-haiku-4-5}" +MODEL_CODEX="${HIVE_SMOKE_MODEL_CODEX:-gpt-5.4-mini}" + +# shellcheck source=../config/backends.conf disable=SC1091 +source "$ROOT/config/backends.conf" + +echo "=== backend smoke: contributor CLI integration (A: drift, S: wire, B: live) ===" + +# ── A. Static drift checks ─────────────────────────────────────────────────── +echo "" +echo "-- A1: HEADLESS_BACKENDS (relay) agrees with KNOWN_BACKENDS (backends.conf) --" +# Extract the real table from the shipped relay source, the same +# read-the-shipped-artifact technique src/deploy/test_entrypoint_*.sh use on +# entrypoint.sh. A backend added to either list without a headless decision +# now fails a test instead of drifting silently. +HEADLESS_KEYS="$(sed -n '/^const HEADLESS_BACKENDS = {$/,/^};$/p' "$RELAY" \ + | grep -E '^ [a-z]+: \{' | sed -E 's/^ ([a-z]+):.*/\1/')" +if [ -z "$HEADLESS_KEYS" ]; then + fail "HEADLESS_BACKENDS table extracted from bin/contributor-relay.sh" \ + "the sed/grep anchors matched nothing — did the table's formatting change?" +else + pass "HEADLESS_BACKENDS table extracted ($(echo "$HEADLESS_KEYS" | wc -l) backends)" + for k in $HEADLESS_KEYS; do + case " $KNOWN_BACKENDS " in + *" $k "*) pass "headless backend '$k' is in KNOWN_BACKENDS" ;; + *) fail "headless backend '$k' is in KNOWN_BACKENDS" \ + "relay lists '$k' but config/backends.conf KNOWN_BACKENDS does not" ;; + esac + done + # The complement is a decision, not an accident: bob and aider drive an + # interactive TUI with no one-shot entry point (see the HEADLESS_BACKENDS + # header comment). A new backend landing in this list means someone added it + # to backends.conf without deciding its headless story. + NON_HEADLESS="" + for k in $KNOWN_BACKENDS; do + case " +$HEADLESS_KEYS +" in + *" +$k +"*) ;; + *) NON_HEADLESS="$NON_HEADLESS $k" ;; + esac + done + check "backends without a headless mode are exactly the documented pair" \ + "aider bob" "$(echo "$NON_HEADLESS" | tr ' ' '\n' | grep -v '^$' | sort | tr '\n' ' ' | sed 's/ $//')" +fi + +echo "" +echo "-- A2/A3: CLI version pins agree across both Dockerfiles --" +pin() { grep -m1 "^ARG $2=" "$ROOT/$1" | cut -d= -f2; } +check "codex pin: src/Dockerfile == src/Dockerfile.contributor" \ + "$(pin src/Dockerfile CODEX_VERSION)" "$(pin src/Dockerfile.contributor CODEX_VERSION)" +check "claude pin: src/Dockerfile == src/Dockerfile.contributor" \ + "$(pin src/Dockerfile CLAUDE_CODE_VERSION)" "$(pin src/Dockerfile.contributor CLAUDE_CODE_VERSION)" + +# ── Shared rig for S and B ─────────────────────────────────────────────────── +RIG_OK=1 +for tool in node npm jq python3; do + if ! command -v "$tool" >/dev/null 2>&1; then + skip "'$tool' not installed; wire-contract and live scenarios cannot run" + RIG_OK=0 + fi +done + +WORK="" +FAKEHUB_PID="" +RELAY_PID="" +TMUX_SESS="" +cleanup() { + [ -n "$RELAY_PID" ] && kill "$RELAY_PID" 2>/dev/null + [ -n "$FAKEHUB_PID" ] && kill "$FAKEHUB_PID" 2>/dev/null + [ -n "$TMUX_SESS" ] && tmux kill-session -t "$TMUX_SESS" 2>/dev/null + [ -n "$WORK" ] && rm -rf "$WORK" +} +trap cleanup EXIT + +if [ "$RIG_OK" = "1" ]; then + WORK="$(mktemp -d)" + + # The relay's one npm dependency. Same install-into-scratch + NODE_PATH shape + # the Justfile relay recipe and src/Dockerfile.contributor use. Resolution is + # checked FROM $WORK: the fake hub script lives there, and a ws that only + # resolves from the repo root (a stray node_modules in the checkout) would + # pass a cwd-based check and still leave the hub unable to start. + if ! (cd "$WORK" && node -e "require('ws')" 2>/dev/null); then + (cd "$WORK" && npm install --no-fund --no-audit ws >/dev/null 2>&1) + export NODE_PATH="$WORK/node_modules${NODE_PATH:+:$NODE_PATH}" + fi + if ! (cd "$WORK" && node -e "require('ws')" 2>/dev/null); then + skip "npm install ws failed; wire-contract and live scenarios cannot run" + RIG_OK=0 + fi +fi + +if [ "$RIG_OK" = "1" ]; then + # A fake hub speaking the five message types the relay needs (the ws port of + # src/deploy/test_contribute_move.sh's fakehub.py): challenge, accept auth, + # assign one task with SMOKE_PROMPT verbatim, answer pings, and append every + # inbound relay message as JSONL for the suite to assert on. + cat > "$WORK/fakehub-ws.js" <<'JS' +const WebSocket = require('ws'); +const fs = require('fs'); +const portFile = process.argv[2]; +const logFile = process.argv[3]; +const prompt = process.env.SMOKE_PROMPT || 'missing SMOKE_PROMPT'; +let done = false; +const wss = new WebSocket.Server({ host: '127.0.0.1', port: 0 }, () => { + fs.writeFileSync(portFile, String(wss.address().port)); +}); +wss.on('connection', (ws) => { + const send = (o) => { try { ws.send(JSON.stringify(o)); } catch (_) {} }; + let assigned = false; + send({ type: 'auth_challenge' }); + ws.on('message', (data) => { + let msg; + try { msg = JSON.parse(data.toString()); } catch (_) { return; } + fs.appendFileSync(logFile, JSON.stringify(msg) + '\n'); + if (msg.type === 'auth_response') { + send({ type: 'auth_ok', contributor_id: 'smoke', trust_tier: 'contributor' }); + } else if (msg.type === 'ready' && !assigned) { + assigned = true; + send({ type: 'task_assign', task_id: 'smoke-1', task_gen: 1, kind: 'issue', + repo: 'kubestellar/hive', number: 0, title: 'backend smoke', prompt }); + } else if (msg.type === 'ping') { + send({ type: 'pong' }); + } else if (msg.type === 'task_complete' || msg.type === 'task_failed') { + done = true; + setTimeout(() => process.exit(0), 500); + } + }); +}); +setTimeout(() => process.exit(done ? 0 : 3), + Number(process.env.SMOKE_HUB_TIMEOUT_MS || 600000)); +JS +fi + +# start_fakehub NAME — starts a hub instance; sets HUB_PORT/HUB_LOG/FAKEHUB_PID. +start_fakehub() { + local name="$1" portfile="$WORK/hub-$1.port" + HUB_LOG="$WORK/hub-$name.jsonl" + rm -f "$portfile" "$HUB_LOG" + SMOKE_PROMPT="$SMOKE_PROMPT" SMOKE_HUB_TIMEOUT_MS="${SMOKE_HUB_TIMEOUT_MS:-600000}" \ + node "$WORK/fakehub-ws.js" "$portfile" "$HUB_LOG" >"$WORK/hub-$name.out" 2>&1 & + FAKEHUB_PID=$! + for _ in $(seq 1 50); do + [ -s "$portfile" ] && { HUB_PORT="$(cat "$portfile")"; return 0; } + sleep 0.1 + done + echo " fake hub '$name' never wrote its port" >&2 + return 1 +} + +# wait_for_terminal LOG DEADLINE_S — waits for a task_complete or task_failed +# to land in the hub's JSONL. +wait_for_terminal() { + local log="$1" deadline="$2" start=$SECONDS + while [ $((SECONDS - start)) -lt "$deadline" ]; do + if [ -f "$log" ] && jq -e -s \ + 'map(select(.type == "task_complete" or .type == "task_failed")) | length > 0' \ + "$log" >/dev/null 2>&1; then + return 0 + fi + sleep 2 + done + return 1 +} + +# msg_field LOG TYPE JQ_EXPR — extracts a field from the first message of TYPE. +msg_field() { + jq -r -s --arg t "$2" "map(select(.type == \$t)) | first | $3" "$1" 2>/dev/null +} + +msg_seen() { + jq -e -s --arg t "$2" 'map(select(.type == $t)) | length > 0' "$1" >/dev/null 2>&1 +} + +# dump_evidence NAME RELAY_LOG — the escalation-package rule: a red run must +# carry its own raw evidence into the workflow log, so the filed issue's linked +# run answers "what did it actually print" without a rerun. +dump_evidence() { + echo " ---- evidence: $1 ----" + [ -n "${2:-}" ] && [ -f "$2" ] && tail -n 30 "$2" | sed 's/^/ relay| /' + [ -f "${HUB_LOG:-}" ] && tail -n 10 "$HUB_LOG" | sed 's/^/ hub | /' + if [ -n "$TMUX_SESS" ]; then + tmux capture-pane -t "$TMUX_SESS" -p 2>/dev/null | tail -n 20 | sed 's/^/ pane | /' + fi + echo " ---- end evidence ----" +} + +stop_scenario() { + [ -n "$RELAY_PID" ] && kill "$RELAY_PID" 2>/dev/null + [ -n "$FAKEHUB_PID" ] && kill "$FAKEHUB_PID" 2>/dev/null + RELAY_PID="" + FAKEHUB_PID="" + if [ -n "$TMUX_SESS" ]; then + tmux kill-session -t "$TMUX_SESS" 2>/dev/null + TMUX_SESS="" + fi + wait 2>/dev/null +} + +# The one prompt every live scenario sends. It IS the sentinel contract from +# buildTaskPromptBody (src/pkg/dashboard/contribute_ws.go) in miniature: one +# model turn, no file access, and the exact HIVE_VERDICT line +# detectHiveVerdict() parses. A backend that cannot follow this has broken the +# completion contract — precisely what the suite exists to catch. +SMOKE_PROMPT='This is an automated integration check. Do not read, create, or modify any files. Reply with exactly this single line and nothing else: HIVE_VERDICT: no_work_needed — backend smoke' + +# run_relay_headless BACKEND MODEL HOME_DIR EXTRA_PATH — starts the real relay +# in headless mode against the current fake hub. EXTRA_PATH prepends a stub +# directory (S scenarios); empty for real CLIs. +run_relay_headless() { + local b="$1" model="$2" home="$3" extra_path="$4" + RELAY_LOG="$WORK/relay-$b-headless.log" + mkdir -p "$home" "$WORK/ws-$b" + ( + cd "$ROOT" || exit 1 + [ -n "$extra_path" ] && export PATH="$extra_path:$PATH" + HOME="$home" \ + AGENT_BACKEND="$b" \ + AGENT_MODEL="$model" \ + CONTRIBUTOR_MODE=headless \ + HIVE_HUB="ws://127.0.0.1:$HUB_PORT/contribute" \ + HIVE_REGISTRATION_TOKEN=smoke-token \ + HIVE_WORKSPACE_DIR="$WORK/ws-$b" \ + HIVE_HEADLESS_STATUS_FILE="$WORK/status-$b.json" \ + HIVE_HEADLESS_TASK_TIMEOUT_MS=300000 \ + HIVE_TASK_FILE="$WORK/task-$b.json" \ + HIVE_GH_TOKEN_CACHE="$WORK/gh-$b.cache" \ + exec node "$RELAY" + ) >"$RELAY_LOG" 2>&1 & + RELAY_PID=$! +} + +# ── S. Stub wire-contract scenarios (keyless, no API spend) ────────────────── +if [ "$RIG_OK" = "1" ]; then + echo "" + echo "-- S1: stub backend, happy path — task_complete carries the verdict --" + STUB="$WORK/stub-ok" + mkdir -p "$STUB" + # A stand-in "claude" that behaves like a compliant one-shot CLI: prints the + # sentinel and exits 0. Everything else in the loop — handshake, assignment, + # execFile, verdict parsing, wire reporting — is the real relay. + printf '#!/bin/sh\necho "HIVE_VERDICT: no_work_needed — stub smoke"\nexit 0\n' > "$STUB/claude" + chmod +x "$STUB/claude" + if start_fakehub s1; then + run_relay_headless claude "" "$WORK/home-s1" "$STUB" + if wait_for_terminal "$HUB_LOG" 60; then + if msg_seen "$HUB_LOG" task_accepted; then pass "relay accepted the task"; else fail "relay accepted the task"; fi + check "task_complete result" "completed" "$(msg_field "$HUB_LOG" task_complete .result)" + check "verdict parsed off the one-shot output" "no_work_needed" \ + "$(msg_field "$HUB_LOG" task_complete .verdict)" + check "relay re-advertised ready after completing" "ready" \ + "$(jq -r -s 'last | .type' "$HUB_LOG")" + check "headless status file settled" "waiting" \ + "$(jq -r '.state' "$WORK/status-claude.json" 2>/dev/null)" + else + fail "stub happy path reached a terminal message within 60s" + dump_evidence "S1" "$RELAY_LOG" + fi + else + fail "fake hub started (S1)" + fi + stop_scenario + + echo "" + echo "-- S2: stub backend, failure path — task_failed carries the exit code --" + STUB_BAD="$WORK/stub-bad" + mkdir -p "$STUB_BAD" + printf '#!/bin/sh\necho "stub detonation" >&2\nexit 42\n' > "$STUB_BAD/claude" + chmod +x "$STUB_BAD/claude" + if start_fakehub s2; then + run_relay_headless claude "" "$WORK/home-s2" "$STUB_BAD" + if wait_for_terminal "$HUB_LOG" 60; then + check "task_failed result" "failed" "$(msg_field "$HUB_LOG" task_failed .result)" + check "failure is not permanent (retryable elsewhere)" "false" \ + "$(msg_field "$HUB_LOG" task_failed .permanent)" + contains "reason names the exit code" \ + "$(msg_field "$HUB_LOG" task_failed .reason)" "code 42" + contains "reason preserves the CLI's own last line" \ + "$(msg_field "$HUB_LOG" task_failed .reason)" "stub detonation" + else + fail "stub failure path reached a terminal message within 60s" + dump_evidence "S2" "$RELAY_LOG" + fi + else + fail "fake hub started (S2)" + fi + stop_scenario +fi + +# ── B. Live per-backend scenarios ──────────────────────────────────────────── + +# seed_backend_auth BACKEND HOME_DIR — puts a credential into a throwaway HOME. +# Prefers the API-key path (what the scheduled lane uses); falls back to +# copying the operator's own logged-in credential so a maintainer's laptop run +# exercises the full arm too. Echoes "ok" or "missing". +seed_backend_auth() { + local b="$1" home="$2" + mkdir -p "$home" + case "$b" in + claude) + if [ -n "${ANTHROPIC_API_KEY:-}" ]; then + seeded=ok + elif [ -f "$REAL_HOME/.claude/.credentials.json" ]; then + mkdir -p "$home/.claude" + cp "$REAL_HOME/.claude/.credentials.json" "$home/.claude/" + chmod 600 "$home/.claude/.credentials.json" 2>/dev/null || true + seeded=ok + else + seeded=missing + fi + if [ "$seeded" = "ok" ]; then + # Pre-answer every first-run gate a fresh HOME raises, the way the + # hub does (inferenceUserConfigSeed / inferenceSettingsSeed in + # src/pkg/agent/manager.go): onboarding + custom-API-key approval in + # .claude.json, and — crucially — skipDangerousModePermissionPrompt + # in .claude/settings.json, the only key that suppresses the "Bypass + # Permissions mode" consent menu, whose default selection is + # "No, exit". Without it, any dismissal loop that answers with a bare + # Enter makes claude EXIT and the pane degrade to bash. + HOME="$home" python3 - <<'PYEOF' 2>/dev/null || true +import json, os +home = os.path.expanduser('~') +d = {'hasCompletedOnboarding': True, 'autoUpdates': False, 'installMethod': 'npm', + 'bypassPermissionsModeAccepted': True} +key = os.environ.get('ANTHROPIC_API_KEY', '') +if key: + d['customApiKeyResponses'] = {'approved': [k for k in (key, key[-20:]) if k], 'rejected': []} +with open(os.path.join(home, '.claude.json'), 'w') as f: + json.dump(d, f, indent=2) +os.makedirs(os.path.join(home, '.claude'), exist_ok=True) +with open(os.path.join(home, '.claude', 'settings.json'), 'w') as f: + json.dump({'permissions': {'allow': [], 'deny': []}, + 'hasCompletedOnboarding': True, + 'bypassPermissions': True, + 'hasAcknowledgedDisclaimer': True, + 'skipDangerousModePermissionPrompt': True}, f, indent=2) +PYEOF + chmod 600 "$home/.claude.json" 2>/dev/null || true + fi + echo "$seeded" + ;; + codex) + # A minimal CODEX_HOME under the throwaway HOME. This deliberately walks + # the fresh-CODEX_HOME surface the #5335 healing work covers, and the + # auth.json shape matches codex_auth_json_has_credentials in + # bin/contributor-agent.sh. + if [ -n "${OPENAI_API_KEY:-}" ]; then + mkdir -p "$home/.codex" + printf '{"OPENAI_API_KEY": "%s"}\n' "$OPENAI_API_KEY" > "$home/.codex/auth.json" + chmod 600 "$home/.codex/auth.json" + echo ok + elif [ -f "$REAL_HOME/.codex/auth.json" ]; then + mkdir -p "$home/.codex" + cp "$REAL_HOME/.codex/auth.json" "$home/.codex/" + chmod 600 "$home/.codex/auth.json" 2>/dev/null || true + echo ok + else + echo missing + fi + ;; + *) + echo missing + ;; + esac +} + +smoke_model_for() { + case "$1" in + claude) normalize_model_for_backend claude "$MODEL_CLAUDE" ;; + codex) echo "$MODEL_CODEX" ;; + *) echo "" ;; + esac +} + +if [ "$RIG_OK" = "1" ]; then + for b in $SMOKE_BACKENDS; do + model="$(smoke_model_for "$b")" + bhome="$WORK/home-$b" + + echo "" + echo "-- B0 [$b]: detect_cli health probe --" + # contributor-agent.sh's own detection, through its own test seam — the + # probe every backend gets, normalizing the old asymmetry where only the + # claude arm of `just contribute-check` did a real check. + st="$(HOME="$bhome" HIVE_REGISTRATION_TOKEN=smoke-token \ + HIVE_CONTRIBUTOR_AGENT_TEST_DETECT_CLI=1 AGENT_BACKEND="$b" \ + bash "$ROOT/bin/contributor-agent.sh" 2>/dev/null | tail -1)" + case "$st" in + OK) pass "detect_cli($b) = OK" ;; + NOT_INSTALLED) + skip "$b CLI not installed — live scenarios for '$b' cannot run" + continue + ;; + *) + # NOT_AUTHED/BROKEN: report it, and let the credential check below + # decide whether the live scenarios can still run (detect_cli reads + # the real HOME's login state; the scenarios bring their own key). + echo " note: detect_cli($b) = ${st:-}" + ;; + esac + + if [ "$(seed_backend_auth "$b" "$bhome")" != "ok" ]; then + skip "no credential for '$b' (need ANTHROPIC_API_KEY / OPENAI_API_KEY or an existing login) — live scenarios skipped" + continue + fi + + echo "" + echo "-- B1 [$b]: headless end-to-end (real CLI one-shot, $model) --" + if start_fakehub "b1-$b"; then + run_relay_headless "$b" "$model" "$bhome" "" + if wait_for_terminal "$HUB_LOG" 360; then + if msg_seen "$HUB_LOG" task_accepted; then pass "relay accepted the task"; else fail "relay accepted the task"; fi + if msg_seen "$HUB_LOG" task_complete; then + check "task_complete result" "completed" "$(msg_field "$HUB_LOG" task_complete .result)" + check "the $b CLI followed the sentinel contract" "no_work_needed" \ + "$(msg_field "$HUB_LOG" task_complete .verdict)" + else + fail "headless run completed" \ + "task_failed: $(msg_field "$HUB_LOG" task_failed .reason)" + dump_evidence "B1-$b" "$RELAY_LOG" + fi + else + fail "headless run reached a terminal message within 360s" + dump_evidence "B1-$b" "$RELAY_LOG" + fi + else + fail "fake hub started (B1-$b)" + fi + stop_scenario + + if ! command -v tmux >/dev/null 2>&1; then + skip "tmux not installed — interactive scenario for '$b' skipped" + continue + fi + + echo "" + echo "-- B2 [$b]: interactive end-to-end (tmux pane, completion_signal) --" + # The drift surface: readiness regexes against a REAL current pane, + # first-run dialog auto-dismissal, and the #5376 completion contract — + # completion_signal must be 'verdict'. chrome_idle here means the task + # completed only because the fallback saved it: the sentinel contract is + # broken for this backend and the fleet is one chrome restyle away from + # the next #4127. + TMUX_SESS="hive-smoke-$b" + tmux kill-session -t "$TMUX_SESS" 2>/dev/null + if start_fakehub "b2-$b" && tmux new-session -d -s "$TMUX_SESS" -c "$WORK/ws-$b"; then + CMD="$(backend_binary "$b")" + PERM_FLAG="$(backend_perm_flag_shell "$b")" + MODEL_FLAG="" + case "$b" in goose|bob) ;; *) [ -n "$model" ] && MODEL_FLAG="--model $model" ;; esac + # Same launch line contributor-agent.sh types, into the same kind of + # fresh-HOME pane a new contributor gets. + tmux send-keys -t "$TMUX_SESS" \ + "cd $(printf %q "$WORK/ws-$b") && HOME=$(printf %q "$bhome") CODEX_HOME=$(printf %q "$bhome/.codex") $CMD $PERM_FLAG $MODEL_FLAG" Enter + + # contributor-agent.sh's auto-dismiss loop, abbreviated: first-run + # trust/theme/API-key dialogs must be cleared for readiness to be + # reachable at all — their patterns going stale is itself a drift + # failure this scenario would surface as a readiness timeout. + ( + for _ in $(seq 1 10); do + sleep 3 + PANE="$(tmux capture-pane -t "$TMUX_SESS" -p -S -10 2>/dev/null || true)" + if echo "$PANE" | grep -q "trust this folder\|trust the files\|Confirm folder trust\|Enter to confirm"; then + tmux send-keys -t "$TMUX_SESS" Enter 2>/dev/null || true + elif echo "$PANE" | grep -q "Do you trust the contents of this directory"; then + tmux send-keys -t "$TMUX_SESS" "1" Enter 2>/dev/null || true + elif echo "$PANE" | grep -q "Choose the text style"; then + tmux send-keys -t "$TMUX_SESS" "1" Enter 2>/dev/null || true + elif echo "$PANE" | grep -q "Bypass Permissions mode"; then + # Fallback only — the settings seed suppresses this menu. Its + # default selection is "No, exit", so a bare Enter kills the CLI. + tmux send-keys -t "$TMUX_SESS" "2" Enter 2>/dev/null || true + elif echo "$PANE" | grep -qi "custom API key"; then + tmux send-keys -t "$TMUX_SESS" "1" Enter 2>/dev/null || true + elif echo "$PANE" | grep -q "bypass permissions\|❯\|›\|/ commands\|> *$"; then + break + fi + done + ) & + DISMISS_PID=$! + + RELAY_LOG="$WORK/relay-$b-interactive.log" + ( + cd "$ROOT" || exit 1 + HOME="$bhome" \ + CODEX_HOME="$bhome/.codex" \ + AGENT_BACKEND="$b" \ + AGENT_MODEL="$model" \ + HIVE_AGENT_SESSION="$TMUX_SESS" \ + HIVE_AGENT_CWD="$WORK/ws-$b" \ + HIVE_HUB="ws://127.0.0.1:$HUB_PORT/contribute" \ + HIVE_REGISTRATION_TOKEN=smoke-token \ + HIVE_WORKSPACE_DIR="$WORK/ws-$b" \ + HIVE_TASK_FILE="$WORK/task-$b.json" \ + HIVE_GH_TOKEN_CACHE="$WORK/gh-$b.cache" \ + exec node "$RELAY" + ) >"$RELAY_LOG" 2>&1 & + RELAY_PID=$! + + # The interactive completion check runs on the relay's 120s progress + # tick, so the floor here is ~2.5 minutes even for an instant reply. + if wait_for_terminal "$HUB_LOG" 480; then + if msg_seen "$HUB_LOG" task_complete; then + sig="$(msg_field "$HUB_LOG" task_complete .completion_signal)" + if [ "$sig" = "verdict" ]; then + pass "completion_signal=verdict — the $b CLI honored the sentinel contract" + else + fail "completion_signal=verdict — the $b CLI honored the sentinel contract" \ + "got '$sig': the task completed but only the chrome-idle fallback saved it; the HIVE_VERDICT contract is broken for $b" + dump_evidence "B2-$b" "$RELAY_LOG" + fi + check "verdict on the wire" "no_work_needed" \ + "$(msg_field "$HUB_LOG" task_complete .verdict)" + else + fail "interactive run completed" \ + "task_failed: $(msg_field "$HUB_LOG" task_failed .reason)" + dump_evidence "B2-$b" "$RELAY_LOG" + fi + else + fail "interactive run reached a terminal message within 480s (readiness regexes may no longer match the real $b pane)" + dump_evidence "B2-$b" "$RELAY_LOG" + fi + kill "$DISMISS_PID" 2>/dev/null + else + fail "fake hub + tmux session started (B2-$b)" + fi + stop_scenario + done +fi + +echo "" +echo "=== Results: $PASS passed, $FAIL failed ===" +[ "$FAIL" -eq 0 ] diff --git a/src/Dockerfile.contributor b/src/Dockerfile.contributor index 7a89de66f..068aa2e92 100644 --- a/src/Dockerfile.contributor +++ b/src/Dockerfile.contributor @@ -74,10 +74,14 @@ ARG COPILOT_VERSION=1.0.59 # `claude --version` makes the build itself fail loudly if the link is missing, # instead of every contributor task failing at runtime. ARG KILO_CLI_VERSION=7.5.6 +# Same ARG name and value as src/Dockerfile so bin/test_backend_smoke.sh can +# assert the two pins agree — this line was previously an inline hardcode that +# could (and did) drift from the main image's CODEX_VERSION unnoticed. +ARG CODEX_VERSION=0.146.0 RUN npm install -g --ignore-scripts --no-fund --no-audit \ @anthropic-ai/claude-code@${CLAUDE_CODE_VERSION} \ @github/copilot@${COPILOT_VERSION} \ - @openai/codex@0.146.0 \ + @openai/codex@${CODEX_VERSION} \ @kilocode/cli@${KILO_CLI_VERSION} \ && npm cache clean --force \ && node "$(npm root -g)/@anthropic-ai/claude-code/install.cjs" \ From ab67809411ca240a71b6b5117491dca6e31660a0 Mon Sep 17 00:00:00 2001 From: Douglas Baggett Date: Mon, 31 Aug 2026 21:13:17 -0400 Subject: [PATCH 012/190] =?UTF-8?q?=E2=9C=A8=20telemetry:=20durable=20per-?= =?UTF-8?q?run=20contributor=20task=20records=20with=20a=20scenario=20ratc?= =?UTF-8?q?het?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every accepted task_complete/task_failed already carries a normalized closed vocabulary — completion_signal (#5376), the failure kind (#2547), the no_work_needed verdict (#3987) — but none of it survived anywhere an operator could aggregate: the slog line rotates away, activity.json is capped at 50 entries and drops reason/kind, and nothing recorded task durations at all. "Which backend completes on its verdict vs the chrome-idle fallback" was unanswerable for real fleet traffic — the exact question the backend smoke asks synthetically. - src/pkg/dashboard/task_run_log.go: one JSONL record per terminal task report, appended to /data/contributors/task_runs.jsonl (0600, 10 MiB cap with a single .1 rotation). Each record carries the hub-normalized fields plus a derived `scenario` from a closed, stable vocabulary: verdict_complete, idle_complete (the ratchet metric — sentinel non-compliance), headless_complete, env_failure, task_failure, unspecified_failure. Best-effort by contract; DECLARE only — nothing routes, cooldowns, or gates on it, the same boundary contribute_protocol.go draws. - contribute_ws.go: a taskAssignedAt anchor on the connection (separate from lastLeaseRenew, which progress reports move) so records carry real wall-clock duration; appends in the accepted task_complete and task_failed branches, reusing the exact locals the structured log line already assembles. - GET /api/contribute/run-stats: per-backend scenario counts, chrome-idle share, and duration p50/p95 over a trailing window (?days=N, default 7). Public read-only like the sibling /api/contribute* GETs — aggregates only, no usernames, reasons, or tokens. Documented in dashboard/openapi.json (route-parity test enforced). - Session mirrors task_id, the correlation key github.InvocationMeta.Session reserves, so PR trailers can later join back to run records without a format change. Tests: scenario-derivation matrix, append/rotation/permissions, windowed aggregation with torn-line tolerance, and an end-to-end WebSocket task_complete drive asserting one record with positive duration plus the run-stats endpoint seeing it. Full pkg/dashboard suite green under -race. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Jo76R9XXJ2iq6YcmVUiX2u Signed-off-by: Douglas Baggett --- dashboard/openapi.json | 80 +++++++ src/pkg/dashboard/api_contribute.go | 5 + src/pkg/dashboard/contribute_ws.go | 70 ++++++ src/pkg/dashboard/task_run_log.go | 280 ++++++++++++++++++++++++ src/pkg/dashboard/task_run_log_test.go | 286 +++++++++++++++++++++++++ 5 files changed, 721 insertions(+) create mode 100644 src/pkg/dashboard/task_run_log.go create mode 100644 src/pkg/dashboard/task_run_log_test.go diff --git a/dashboard/openapi.json b/dashboard/openapi.json index a9ea4b32c..6da1e6a0d 100644 --- a/dashboard/openapi.json +++ b/dashboard/openapi.json @@ -11094,6 +11094,86 @@ } } }, + "/api/contribute/run-stats": { + "get": { + "tags": [ + "Contribute" + ], + "summary": "Per-backend run-scenario aggregates", + "description": "Aggregates the durable contributor task-run log into per-backend scenario counts (verdict_complete, idle_complete, headless_complete, env_failure, task_failure, unspecified_failure), the chrome-idle share (the completion-sentinel non-compliance rate), and duration percentiles, over a trailing window. Public, GET only, no side effects \u2014 aggregate counts only, no usernames, reasons, or tokens.", + "parameters": [ + { + "name": "days", + "in": "query", + "required": false, + "schema": { + "type": "integer", + "minimum": 0, + "maximum": 365, + "default": 7 + }, + "description": "Trailing window in days; 0 aggregates everything in the live log." + } + ], + "responses": { + "200": { + "description": "Per-backend run-scenario aggregates", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "window_days": { + "type": "integer" + }, + "total": { + "type": "integer" + }, + "backends": { + "type": "array", + "items": { + "type": "object", + "properties": { + "backend": { + "type": "string" + }, + "total": { + "type": "integer" + }, + "completed": { + "type": "integer" + }, + "failed": { + "type": "integer" + }, + "scenarios": { + "type": "object", + "description": "Map of scenario name -> count", + "additionalProperties": { + "type": "integer" + } + }, + "chrome_idle_share": { + "type": "number", + "description": "idle_complete / completed \u2014 the completion-sentinel non-compliance rate to ratchet toward zero" + }, + "duration_p50_s": { + "type": "number" + }, + "duration_p95_s": { + "type": "number" + } + } + } + } + } + } + } + } + } + } + } + }, "/api/contribute/opportunistic": { "get": { "tags": [ diff --git a/src/pkg/dashboard/api_contribute.go b/src/pkg/dashboard/api_contribute.go index cdfbd3548..9df61b5e3 100644 --- a/src/pkg/dashboard/api_contribute.go +++ b/src/pkg/dashboard/api_contribute.go @@ -333,6 +333,11 @@ func (s *Server) registerContributeRoutes() { // reads (only counts + already-public usernames; no tokens, no PII). GET only, // no side effects. See contribute_metrics.go. s.mux.HandleFunc("GET /api/contribute/metrics", s.handleContributeMetrics) + // Read-only per-backend RUN SCENARIOS: aggregates over the durable task-run + // log (task_run_log.go) — scenario counts, sentinel-compliance share, and + // duration percentiles per backend. Public like the other /api/contribute* + // reads (aggregate counts only; no usernames, no reasons, no tokens). + s.mux.HandleFunc("GET /api/contribute/run-stats", s.handleContributeRunStats) // Read-only TRIAGE ladder (#2612 part b): the contribute issues grouped into a // Warp-style lifecycle (Triaging → Ready → Implementing → Reviewing → Closed), // DERIVED LIVE from the ready queue + fleet snapshot + the PR→issue link (part diff --git a/src/pkg/dashboard/contribute_ws.go b/src/pkg/dashboard/contribute_ws.go index 9a9cbfdcf..2ba986d7f 100644 --- a/src/pkg/dashboard/contribute_ws.go +++ b/src/pkg/dashboard/contribute_ws.go @@ -102,6 +102,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 @@ -3577,6 +3583,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. // @@ -3598,6 +3608,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 @@ -3698,6 +3709,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: @@ -3768,6 +3806,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 @@ -3777,6 +3818,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 @@ -3840,6 +3882,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() @@ -5425,6 +5492,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 diff --git a/src/pkg/dashboard/task_run_log.go b/src/pkg/dashboard/task_run_log.go new file mode 100644 index 000000000..dd5acd86b --- /dev/null +++ b/src/pkg/dashboard/task_run_log.go @@ -0,0 +1,280 @@ +package dashboard + +// Durable per-run contributor task telemetry (the DECLARE half only). +// +// Every accepted task_complete / task_failed already carries a small closed +// vocabulary — completion_signal (#5376), the task-failure kind (#2547), the +// no_work_needed verdict (#3987) — but until now none of it survived anywhere +// an operator could aggregate: the slog line rotates away, activity.json is +// capped at maxActivityEntries and drops reason/kind entirely, and nothing +// records how long a run took. So "which backend completes on its verdict vs +// the chrome-idle fallback" and "which backends fail on their environment" — +// the exact questions the backend smoke (bin/test_backend_smoke.sh) asks +// synthetically — were unanswerable for real fleet traffic. +// +// This file appends one JSONL record per terminal task report to +// /data/contributors/task_runs.jsonl and derives a predefined `scenario` from +// the already-normalized fields, so the record set can be ratcheted over time +// (watch idle_complete and env_failure trend down per backend). +// +// DECLARE, never ROUTE: nothing here influences cooldowns, trust, routing, or +// offers — the same boundary contribute_protocol.go draws for the failure +// kind. Writes are best-effort; a telemetry failure never fails a task. + +import ( + "encoding/json" + "net/http" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + "sync" + "time" +) + +// taskRunLogPath is where terminal task reports are appended, one JSON object +// per line. A var (not const) so tests point it at a scratch file. +var taskRunLogPath = "/data/contributors/task_runs.jsonl" + +// taskRunLogMaxBytes bounds the live file. On overflow the file is rotated to +// a single ".1" predecessor (replacing any previous one), so disk use is +// capped at ~2× this regardless of fleet size or uptime. +const taskRunLogMaxBytes = 10 << 20 + +var taskRunMu sync.Mutex + +// The predefined scenario vocabulary. Closed and stable by design: these +// strings are the ratchet axis, so week-over-week comparison depends on their +// spelling never changing. Extend by adding values; never rename. +const ( + // scenarioVerdictComplete: the agent ended the task with its own + // HIVE_VERDICT sentinel — the completion contract working as designed. + scenarioVerdictComplete = "verdict_complete" + // scenarioIdleComplete: the task completed only because the chrome-idle + // fallback fired — the sentinel contract is NOT being honored for this + // backend. This is the primary ratchet metric. + scenarioIdleComplete = "idle_complete" + // scenarioHeadlessComplete: completed with no completion_signal on the + // wire — the headless one-shot path (whose completion IS the exit code), + // or a relay predating #5376. + scenarioHeadlessComplete = "headless_complete" + // scenarioEnvFailure: the client's runtime could not run the work — the + // broken-backend-integration class the backend smoke exists to catch. + scenarioEnvFailure = "env_failure" + // scenarioTaskFailure: the work was attempted and failed on its merits. + scenarioTaskFailure = "task_failure" + // scenarioUnspecifiedFailure: a failure with no usable kind (older relay, + // or an unrecognized value). + scenarioUnspecifiedFailure = "unspecified_failure" +) + +// TaskRunRecord is one terminal task report, flattened to the fields an +// operator aggregates on. All enum-ish fields hold hub-NORMALIZED values +// (normalizeCompletionSignal, NormalizeTaskFailureKind, +// normalizeCompletionVerdict) — client free text never lands here except the +// bounded failure reason, which the fleet view already displays as-is. +type TaskRunRecord struct { + TS string `json:"ts"` + TaskID string `json:"task_id"` + TaskGen uint64 `json:"task_gen,omitempty"` + Repo string `json:"repo,omitempty"` + Number int `json:"number,omitempty"` + Username string `json:"username"` + Backend string `json:"backend"` + Provider string `json:"provider,omitempty"` + Model string `json:"model,omitempty"` + Effort string `json:"effort,omitempty"` + Role string `json:"role,omitempty"` + // Outcome is "completed" or "failed" — which terminal message arrived. + Outcome string `json:"outcome"` + CompletionSignal string `json:"completion_signal,omitempty"` + Verdict string `json:"verdict,omitempty"` + VerdictReason string `json:"verdict_reason,omitempty"` + FailureKind string `json:"failure_kind,omitempty"` + Reason string `json:"reason,omitempty"` + Permanent bool `json:"permanent,omitempty"` + DurationS float64 `json:"duration_s,omitempty"` + PRURL string `json:"pr_url,omitempty"` + PRVerified bool `json:"pr_verified,omitempty"` + Scenario string `json:"scenario"` + // Session mirrors TaskID for now: the correlation key reserved by + // github.InvocationMeta.Session, so a PR trailer can one day join back to + // this record without a format change. + Session string `json:"session,omitempty"` +} + +// deriveScenario maps a terminal report's normalized fields onto the closed +// scenario vocabulary above. Pure; table-tested. +func deriveScenario(outcome, completionSignal, failureKind string) string { + if outcome == "completed" { + switch completionSignal { + case completionSignalVerdict: + return scenarioVerdictComplete + case completionSignalChromeIdle: + return scenarioIdleComplete + default: + return scenarioHeadlessComplete + } + } + switch failureKind { + case TaskFailureKindEnvironment: + return scenarioEnvFailure + case TaskFailureKindTask: + return scenarioTaskFailure + default: + return scenarioUnspecifiedFailure + } +} + +// appendTaskRun stamps, classifies, and appends one record. Best-effort by +// contract: every failure path logs and returns — a task must never fail (or +// block its read loop meaningfully) on telemetry. +func (h *ContributeWSHub) appendTaskRun(rec TaskRunRecord) { + rec.TS = time.Now().UTC().Format(time.RFC3339) + rec.Scenario = deriveScenario(rec.Outcome, rec.CompletionSignal, rec.FailureKind) + if rec.Session == "" { + rec.Session = rec.TaskID + } + data, err := json.Marshal(rec) + if err != nil { + if h != nil && h.logger != nil { + h.logger.Warn("[contribute-ws] task-run record marshal failed", "error", err) + } + return + } + + taskRunMu.Lock() + defer taskRunMu.Unlock() + path := taskRunLogPath + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + if h != nil && h.logger != nil { + h.logger.Warn("[contribute-ws] task-run log directory creation failed", "error", err) + } + return + } + // Rotate BEFORE appending so the live file never exceeds the cap by more + // than one record. + if st, err := os.Stat(path); err == nil && st.Size() >= taskRunLogMaxBytes { + _ = os.Rename(path, path+".1") // replaces any previous .1: bounded at 2 files + } + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) + if err != nil { + if h != nil && h.logger != nil { + h.logger.Warn("[contribute-ws] task-run log open failed", "error", err) + } + return + } + defer f.Close() + if _, err := f.Write(append(data, '\n')); err != nil && h != nil && h.logger != nil { + h.logger.Warn("[contribute-ws] task-run log write failed", "error", err) + } +} + +// taskRunBackendStats is the per-backend aggregate served by +// /api/contribute/run-stats. Aggregates only — no usernames, matching the +// public read-only posture of the other /api/contribute* GETs. +type taskRunBackendStats struct { + Backend string `json:"backend"` + Total int `json:"total"` + Completed int `json:"completed"` + Failed int `json:"failed"` + Scenarios map[string]int `json:"scenarios"` + // ChromeIdleShare is idle_complete / completed — the sentinel + // non-compliance rate, the number to ratchet toward zero. + ChromeIdleShare float64 `json:"chrome_idle_share"` + DurationP50S float64 `json:"duration_p50_s,omitempty"` + DurationP95S float64 `json:"duration_p95_s,omitempty"` +} + +// readTaskRunStats aggregates the live log (rotated history is deliberately +// excluded — the endpoint answers "recently", the files answer "ever") over +// the trailing window. window <= 0 means everything in the file. +func readTaskRunStats(path string, window time.Duration) ([]taskRunBackendStats, int, error) { + taskRunMu.Lock() + data, err := os.ReadFile(path) + taskRunMu.Unlock() + if err != nil { + if os.IsNotExist(err) { + return []taskRunBackendStats{}, 0, nil + } + return nil, 0, err + } + cutoff := "" + if window > 0 { + cutoff = time.Now().UTC().Add(-window).Format(time.RFC3339) + } + byBackend := map[string]*taskRunBackendStats{} + durations := map[string][]float64{} + total := 0 + for _, line := range strings.Split(string(data), "\n") { + if strings.TrimSpace(line) == "" { + continue + } + var rec TaskRunRecord + if err := json.Unmarshal([]byte(line), &rec); err != nil { + continue // a torn tail line must not poison the aggregate + } + if cutoff != "" && rec.TS < cutoff { // RFC3339 is lexically sortable + continue + } + total++ + b := rec.Backend + if b == "" { + b = "unknown" + } + st := byBackend[b] + if st == nil { + st = &taskRunBackendStats{Backend: b, Scenarios: map[string]int{}} + byBackend[b] = st + } + st.Total++ + st.Scenarios[rec.Scenario]++ + if rec.Outcome == "completed" { + st.Completed++ + } else { + st.Failed++ + } + if rec.DurationS > 0 { + durations[b] = append(durations[b], rec.DurationS) + } + } + out := make([]taskRunBackendStats, 0, len(byBackend)) + for b, st := range byBackend { + if st.Completed > 0 { + st.ChromeIdleShare = float64(st.Scenarios[scenarioIdleComplete]) / float64(st.Completed) + } + if ds := durations[b]; len(ds) > 0 { + sort.Float64s(ds) + st.DurationP50S = ds[len(ds)/2] + st.DurationP95S = ds[(len(ds)*95)/100] + } + out = append(out, *st) + } + sort.Slice(out, func(i, j int) bool { return out[i].Backend < out[j].Backend }) + return out, total, nil +} + +// handleContributeRunStats serves GET /api/contribute/run-stats: per-backend +// scenario counts, completion-signal compliance, and duration percentiles over +// a trailing window (?days=N, default 7, 0 = everything in the live log). +// Public read-only like the sibling /api/contribute* GETs — aggregates only, +// no usernames, no reasons, no tokens. +func (s *Server) handleContributeRunStats(w http.ResponseWriter, r *http.Request) { + days := 7 + if v := r.URL.Query().Get("days"); v != "" { + if n, err := strconv.Atoi(v); err == nil && n >= 0 && n <= 365 { + days = n + } + } + stats, total, err := readTaskRunStats(taskRunLogPath, time.Duration(days)*24*time.Hour) + if err != nil { + http.Error(w, "task-run log unreadable", http.StatusInternalServerError) + return + } + jsonResponse(w, map[string]any{ + "window_days": days, + "total": total, + "backends": stats, + }) +} diff --git a/src/pkg/dashboard/task_run_log_test.go b/src/pkg/dashboard/task_run_log_test.go new file mode 100644 index 000000000..e447f421a --- /dev/null +++ b/src/pkg/dashboard/task_run_log_test.go @@ -0,0 +1,286 @@ +package dashboard + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + "time" + + "github.com/gorilla/websocket" +) + +// Redirects the run log to a scratch file for one test. +func scratchRunLog(t *testing.T) string { + t.Helper() + prev := taskRunLogPath + taskRunLogPath = t.TempDir() + "/task_runs.jsonl" + t.Cleanup(func() { taskRunLogPath = prev }) + return taskRunLogPath +} + +// The scenario vocabulary is the ratchet axis: every (outcome, signal, kind) +// combination must map deterministically, and unknown inputs must land in a +// bucket rather than invent a new one. +func TestDeriveScenario_Table(t *testing.T) { + cases := []struct { + outcome, signal, kind, want string + }{ + {"completed", completionSignalVerdict, "", scenarioVerdictComplete}, + {"completed", completionSignalChromeIdle, "", scenarioIdleComplete}, + // No signal on the wire: the headless one-shot path, or a pre-#5376 + // relay — both normalize to "unknown" before reaching the log. + {"completed", completionSignalUnknown, "", scenarioHeadlessComplete}, + {"completed", "", "", scenarioHeadlessComplete}, + {"failed", "", TaskFailureKindEnvironment, scenarioEnvFailure}, + {"failed", "", TaskFailureKindTask, scenarioTaskFailure}, + {"failed", "", TaskFailureKindUnspecified, scenarioUnspecifiedFailure}, + {"failed", "", "", scenarioUnspecifiedFailure}, + // A failure's stray completion signal must not smuggle it into a + // completion scenario. + {"failed", completionSignalVerdict, TaskFailureKindTask, scenarioTaskFailure}, + } + for _, tc := range cases { + if got := deriveScenario(tc.outcome, tc.signal, tc.kind); got != tc.want { + t.Errorf("deriveScenario(%q,%q,%q) = %q, want %q", + tc.outcome, tc.signal, tc.kind, got, tc.want) + } + } +} + +func TestTaskRunLog_AppendStampsAndDefaults(t *testing.T) { + path := scratchRunLog(t) + var hub *ContributeWSHub // nil hub must be tolerated (best-effort contract) + hub.appendTaskRun(TaskRunRecord{ + TaskID: "ct-1", + Username: "alice", + Backend: "claude", + Outcome: "completed", + CompletionSignal: completionSignalVerdict, + DurationS: 12.5, + }) + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read log: %v", err) + } + var rec TaskRunRecord + if err := json.Unmarshal(data, &rec); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if rec.TS == "" { + t.Error("append must stamp ts") + } + if rec.Scenario != scenarioVerdictComplete { + t.Errorf("scenario = %q, want %q", rec.Scenario, scenarioVerdictComplete) + } + if rec.Session != "ct-1" { + t.Errorf("session must default to the task id, got %q", rec.Session) + } + if fi, err := os.Stat(path); err != nil || fi.Mode().Perm() != 0o600 { + t.Errorf("log must be 0600, got %v (err %v)", fi.Mode(), err) + } +} + +func TestTaskRunLog_RotatesAtCap(t *testing.T) { + path := scratchRunLog(t) + // A live file already at the cap must be rotated to .1 before the append, + // and a previous .1 replaced — disk use stays bounded at two files. + if err := os.MkdirAll(t.TempDir(), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, make([]byte, taskRunLogMaxBytes), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path+".1", []byte("old rotation\n"), 0o600); err != nil { + t.Fatal(err) + } + var hub *ContributeWSHub + hub.appendTaskRun(TaskRunRecord{TaskID: "ct-2", Outcome: "failed"}) + + live, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read live log: %v", err) + } + if len(live) >= taskRunLogMaxBytes { + t.Fatalf("live file was not rotated: %d bytes", len(live)) + } + var rec TaskRunRecord + if err := json.Unmarshal(live, &rec); err != nil || rec.TaskID != "ct-2" { + t.Fatalf("fresh record must land in the fresh file: %v %+v", err, rec) + } + if fi, err := os.Stat(path + ".1"); err != nil || fi.Size() != int64(taskRunLogMaxBytes) { + t.Fatalf(".1 must hold the rotated file, got %v (err %v)", fi, err) + } +} + +func TestReadTaskRunStats_Aggregates(t *testing.T) { + path := scratchRunLog(t) + now := time.Now().UTC() + mk := func(age time.Duration, backend, outcome, signal, kind string, dur float64) string { + rec := TaskRunRecord{ + TS: now.Add(-age).Format(time.RFC3339), + TaskID: "x", + Backend: backend, + Outcome: outcome, + Scenario: deriveScenario(outcome, signal, kind), + DurationS: dur, + } + b, _ := json.Marshal(rec) + return string(b) + } + lines := []string{ + mk(time.Hour, "claude", "completed", completionSignalVerdict, "", 100), + mk(time.Hour, "claude", "completed", completionSignalChromeIdle, "", 300), + mk(time.Hour, "claude", "failed", "", TaskFailureKindEnvironment, 0), + mk(time.Hour, "codex", "completed", "", "", 50), + // Outside a 1-day window; must be excluded from a windowed read. + mk(48*time.Hour, "claude", "completed", completionSignalVerdict, "", 10), + `{"torn json`, // a torn tail line must not poison the aggregate + } + if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0o600); err != nil { + t.Fatal(err) + } + + stats, total, err := readTaskRunStats(path, 24*time.Hour) + if err != nil { + t.Fatalf("readTaskRunStats: %v", err) + } + if total != 4 { + t.Fatalf("windowed total = %d, want 4", total) + } + if len(stats) != 2 || stats[0].Backend != "claude" || stats[1].Backend != "codex" { + t.Fatalf("backends wrong: %+v", stats) + } + cl := stats[0] + if cl.Completed != 2 || cl.Failed != 1 { + t.Errorf("claude completed/failed = %d/%d, want 2/1", cl.Completed, cl.Failed) + } + if cl.Scenarios[scenarioIdleComplete] != 1 || cl.Scenarios[scenarioEnvFailure] != 1 { + t.Errorf("claude scenarios wrong: %+v", cl.Scenarios) + } + if cl.ChromeIdleShare != 0.5 { + t.Errorf("claude chrome_idle_share = %v, want 0.5", cl.ChromeIdleShare) + } + if cl.DurationP50S != 300 { // sorted [100 300], len/2 = index 1 + t.Errorf("claude p50 = %v, want 300", cl.DurationP50S) + } + + // Unwindowed read sees everything parseable. + _, allTotal, err := readTaskRunStats(path, 0) + if err != nil || allTotal != 5 { + t.Fatalf("unwindowed total = %d (err %v), want 5", allTotal, err) + } + + // A missing file is an empty aggregate, not an error. + if s, n, err := readTaskRunStats(path+".missing", 0); err != nil || n != 0 || len(s) != 0 { + t.Fatalf("missing file must aggregate to empty: %v %d %v", s, n, err) + } +} + +// Drives the REAL WebSocket task_complete handler end to end (the same +// harness as TestNoWorkVerdict_NeverGrantsTrustCredit) and asserts the run +// log gains exactly one record carrying the normalized fields and a real +// duration — the two things nothing durably recorded before. +func TestTaskRunLog_RecordedOnCompletion(t *testing.T) { + path := scratchRunLog(t) + s, ts := setupWSTest(t) + defer ts.Close() + + body := `{"github_username":"runlog-user"}` + req := httptest.NewRequest(http.MethodPost, "/api/contribute/register", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + s.mux.ServeHTTP(w, req) + var reg map[string]string + if err := json.Unmarshal(w.Body.Bytes(), ®); err != nil { + t.Fatalf("register response: %v", err) + } + + conn, _, err := websocket.DefaultDialer.Dial(wsURL(ts), nil) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + + readMsg(t, conn) // challenge + conn.WriteJSON(WSMessage{Type: "auth_response", RegistrationToken: reg["registration_token"], CLIBackend: "claude", Model: "claude-haiku-4-5"}) + readMsg(t, conn) // auth_ok + + s.statusMu.Lock() + s.status = &StatusPayload{ + Repos: []FrontendRepo{ + { + Name: "repo1", + Full: "myorg/repo1", + ActionableIssues: []any{ + noWorkIssue(91, "runlog issue", time.Now().Add(-24*time.Hour)), + }, + }, + }, + } + s.statusMu.Unlock() + + conn.WriteJSON(WSMessage{Type: "ready", Seq: 2}) + assign := readMsg(t, conn) + if assign.Type != "task_assign" { + t.Fatalf("expected task_assign, got %+v", assign) + } + conn.WriteJSON(WSMessage{ + Type: "task_complete", TaskID: assign.TaskID, TaskGen: assign.TaskGen, + Result: "completed", Verdict: "no_work_needed", VerdictReason: "smoke", + CompletionSignal: "verdict", + }) + time.Sleep(100 * time.Millisecond) + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("run log not written: %v", err) + } + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + if len(lines) != 1 { + t.Fatalf("want exactly one record, got %d: %q", len(lines), data) + } + var rec TaskRunRecord + if err := json.Unmarshal([]byte(lines[0]), &rec); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if rec.Username != "runlog-user" || rec.Backend != "claude" || rec.Model != "claude-haiku-4-5" { + t.Errorf("identity fields wrong: %+v", rec) + } + if rec.Outcome != "completed" || rec.Scenario != scenarioVerdictComplete { + t.Errorf("outcome/scenario wrong: %+v", rec) + } + if rec.Repo != "myorg/repo1" || rec.Number != 91 { + t.Errorf("task identity wrong: %+v", rec) + } + if rec.Verdict != "no_work_needed" { + t.Errorf("verdict = %q, want no_work_needed", rec.Verdict) + } + if rec.DurationS <= 0 { + t.Errorf("duration_s must be positive (assignment→completion), got %v", rec.DurationS) + } + if rec.PRVerified { + t.Errorf("no PR was reported, pr_verified must be false: %+v", rec) + } + + // And the aggregate endpoint sees it. + req2 := httptest.NewRequest(http.MethodGet, "/api/contribute/run-stats?days=1", nil) + w2 := httptest.NewRecorder() + s.mux.ServeHTTP(w2, req2) + if w2.Code != http.StatusOK { + t.Fatalf("run-stats status %d", w2.Code) + } + var resp struct { + Total int `json:"total"` + Backends []taskRunBackendStats `json:"backends"` + } + if err := json.Unmarshal(w2.Body.Bytes(), &resp); err != nil { + t.Fatalf("run-stats response: %v", err) + } + if resp.Total != 1 || len(resp.Backends) != 1 || resp.Backends[0].Backend != "claude" || + resp.Backends[0].Scenarios[scenarioVerdictComplete] != 1 { + t.Fatalf("run-stats aggregate wrong: %+v", resp) + } +} From ae92788c58a87886a82da573df4ec80e7dec8162 Mon Sep 17 00:00:00 2001 From: Douglas Baggett Date: Mon, 31 Aug 2026 21:25:57 -0400 Subject: [PATCH 013/190] =?UTF-8?q?=E2=9C=A8=20smoke:=20accept=20subscript?= =?UTF-8?q?ion=20logins=20as=20the=20scheduled=20lane's=20credential?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Projects without metered API keys can fund the backend smoke from a Claude Pro/Max or ChatGPT account instead: the suite now accepts HIVE_SMOKE_CLAUDE_CREDENTIALS_B64 (base64 ~/.claude/.credentials.json) and HIVE_SMOKE_CODEX_AUTH_B64 (base64 ~/.codex/auth.json), tried after the API key and before the copy-the-operator's-login fallback. Decode is the careful #5103 shape — temp file, discard on failure — so a corrupt secret degrades to a clean credential-missing skip. The workflow forwards the two new secrets with the same per-arm scoping as the keys, and its header documents the accepted trade-offs: OAuth refresh chains rotate so the stored blob goes stale and needs periodic re-capture, and the smoke shares the account's rate limits with its human. API keys remain the recommended form for an unattended schedule. Verified live: the full claude arm runs green with the credential delivered only via the B64 env (empty HOME, no API key), and a corrupt blob falls through to the skip path. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Jo76R9XXJ2iq6YcmVUiX2u Signed-off-by: Douglas Baggett --- .github/workflows/backend-smoke.yml | 29 ++++++++++++++---- bin/test_backend_smoke.sh | 47 +++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/.github/workflows/backend-smoke.yml b/.github/workflows/backend-smoke.yml index 3f85c4df7..5e7275557 100644 --- a/.github/workflows/backend-smoke.yml +++ b/.github/workflows/backend-smoke.yml @@ -19,11 +19,22 @@ # against the CLI versions hive actually ships. A red here means # contributors are broken TODAY. # -# This is the only workflow in the repo holding model credentials -# (ANTHROPIC_API_KEY / OPENAI_API_KEY repo secrets), which is exactly why it is -# its own file: the podman arm64 lane's documented contract is "no secrets", -# and nothing here may be folded into it. Keys are scoped per matrix arm — -# claude arms never see the OpenAI key and vice versa. +# This is the only workflow in the repo holding model credentials, which is +# exactly why it is its own file: the podman arm64 lane's documented contract +# is "no secrets", and nothing here may be folded into it. Credentials are +# scoped per matrix arm — claude arms never see codex credentials and vice +# versa — and come in two forms, either of which is sufficient: +# ANTHROPIC_API_KEY / OPENAI_API_KEY — metered API keys; never expire, the +# preferred form for an unattended schedule. +# HIVE_SMOKE_CLAUDE_CREDENTIALS_B64 / HIVE_SMOKE_CODEX_AUTH_B64 — base64 of +# a subscription login (~/.claude/.credentials.json from Claude Pro/Max, +# ~/.codex/auth.json from a ChatGPT sign-in), for projects funding the +# smoke from a flat-rate account instead. Two caveats, accepted: the OAuth +# refresh chain rotates, so the stored secret goes stale eventually and a +# maintainer re-captures it from a fresh login (the per-lane issue dedupe +# keeps a stale-credential red from spamming the tracker); and the smoke +# shares the account's rate limits with its human, so a heavy usage day +# can red a run without a hive bug. API keys have neither problem. # # Cost control: 4 runs/day × 2 backends × 2 lanes × 2 single-turn calls on the # cheapest model tier (haiku / mini). The suite's live task is one line with no @@ -71,9 +82,13 @@ jobs: backend: [claude, codex] lane: [latest, pinned] env: - # Per-arm key scoping: each backend arm gets only its own vendor's key. + # Per-arm credential scoping: each backend arm gets only its own + # vendor's credential, in whichever form the repo provisioned (see the + # header). The suite prefers the API key when both are set. ANTHROPIC_API_KEY: ${{ matrix.backend == 'claude' && secrets.ANTHROPIC_API_KEY || '' }} OPENAI_API_KEY: ${{ matrix.backend == 'codex' && secrets.OPENAI_API_KEY || '' }} + HIVE_SMOKE_CLAUDE_CREDENTIALS_B64: ${{ matrix.backend == 'claude' && secrets.HIVE_SMOKE_CLAUDE_CREDENTIALS_B64 || '' }} + HIVE_SMOKE_CODEX_AUTH_B64: ${{ matrix.backend == 'codex' && secrets.HIVE_SMOKE_CODEX_AUTH_B64 || '' }} SMOKE_BACKENDS: ${{ github.event_name == 'workflow_dispatch' && inputs.backends || 'claude codex' }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -125,6 +140,8 @@ jobs: -e HIVE_SMOKE_BACKENDS='${{ matrix.backend }}' \ -e ANTHROPIC_API_KEY \ -e OPENAI_API_KEY \ + -e HIVE_SMOKE_CLAUDE_CREDENTIALS_B64 \ + -e HIVE_SMOKE_CODEX_AUTH_B64 \ -v "${GITHUB_WORKSPACE}:/opt/hive-src:ro,Z" \ ghcr.io/kubestellar/hive-contributor:latest \ -lc 'bash /opt/hive-src/bin/test_backend_smoke.sh' |& tee smoke.log diff --git a/bin/test_backend_smoke.sh b/bin/test_backend_smoke.sh index 6bb0453c2..34788e571 100644 --- a/bin/test_backend_smoke.sh +++ b/bin/test_backend_smoke.sh @@ -38,6 +38,11 @@ # inversion, same shape as # HIVE_TEST_REQUIRE_BEHAVIOURAL in # src/deploy/test_entrypoint_*.sh) +# HIVE_SMOKE_CLAUDE_CREDENTIALS_B64 / HIVE_SMOKE_CODEX_AUTH_B64 +# base64 subscription-login blobs, used +# when no API key is set — see +# seed_backend_auth for sources, order, +# and the staleness caveat # # Run: bash bin/test_backend_smoke.sh (keyless: A + S run, B skips) # ANTHROPIC_API_KEY=... bash bin/test_backend_smoke.sh (full claude arm) @@ -375,9 +380,39 @@ fi # ── B. Live per-backend scenarios ──────────────────────────────────────────── # seed_backend_auth BACKEND HOME_DIR — puts a credential into a throwaway HOME. -# Prefers the API-key path (what the scheduled lane uses); falls back to -# copying the operator's own logged-in credential so a maintainer's laptop run -# exercises the full arm too. Echoes "ok" or "missing". +# Three sources, in order of preference: +# 1. an API key (ANTHROPIC_API_KEY / OPENAI_API_KEY) — the scheduled lane's +# first choice: keys never expire mid-schedule; +# 2. a base64-encoded subscription login blob +# (HIVE_SMOKE_CLAUDE_CREDENTIALS_B64 = ~/.claude/.credentials.json, +# HIVE_SMOKE_CODEX_AUTH_B64 = ~/.codex/auth.json) — for projects running +# the lane on a Claude Pro/Max or ChatGPT account instead of metered +# keys. Same shape as HIVE_CLAUDE_CREDENTIALS_B64 (#5103). CAVEAT: these +# are OAuth tokens with rotating refresh chains, so the stored secret +# goes stale after a while and must be re-captured from a fresh login — +# a stale one surfaces as NOT_AUTHED-style live-scenario failures; +# 3. copying the operator's own logged-in credential from the real HOME, so +# a maintainer's laptop run exercises the full arm with zero setup. +# Echoes "ok" or "missing". + +# decode_b64_credential B64_VALUE DEST — the careful-decode shape from +# contributor-agent.sh (#5103): decode to a temp file first and discard on +# failure, so a corrupt secret leaves no half-written credential behind. +decode_b64_credential() { + local b64="$1" dest="$2" tmp + mkdir -p "$(dirname "$dest")" + tmp="$(mktemp "$dest.tmp.XXXXXX")" + if printf '%s' "$b64" | base64 -d > "$tmp" 2>/dev/null && [ -s "$tmp" ]; then + chmod 600 "$tmp" + mv "$tmp" "$dest" + return 0 + fi + rm -f "$tmp" + # stderr: callers run inside command substitution, which would swallow (and + # worse, capture) a stdout note into the ok/missing result. + echo " note: a base64 credential for $dest did not decode to a non-empty file; ignoring it" >&2 + return 1 +} seed_backend_auth() { local b="$1" home="$2" mkdir -p "$home" @@ -385,6 +420,9 @@ seed_backend_auth() { claude) if [ -n "${ANTHROPIC_API_KEY:-}" ]; then seeded=ok + elif [ -n "${HIVE_SMOKE_CLAUDE_CREDENTIALS_B64:-}" ] \ + && decode_b64_credential "$HIVE_SMOKE_CLAUDE_CREDENTIALS_B64" "$home/.claude/.credentials.json"; then + seeded=ok elif [ -f "$REAL_HOME/.claude/.credentials.json" ]; then mkdir -p "$home/.claude" cp "$REAL_HOME/.claude/.credentials.json" "$home/.claude/" @@ -434,6 +472,9 @@ PYEOF printf '{"OPENAI_API_KEY": "%s"}\n' "$OPENAI_API_KEY" > "$home/.codex/auth.json" chmod 600 "$home/.codex/auth.json" echo ok + elif [ -n "${HIVE_SMOKE_CODEX_AUTH_B64:-}" ] \ + && decode_b64_credential "$HIVE_SMOKE_CODEX_AUTH_B64" "$home/.codex/auth.json"; then + echo ok elif [ -f "$REAL_HOME/.codex/auth.json" ]; then mkdir -p "$home/.codex" cp "$REAL_HOME/.codex/auth.json" "$home/.codex/" From 6c3f5d4a1fdee25da201cc38d66010217bff9ed9 Mon Sep 17 00:00:00 2001 From: Douglas Baggett Date: Mon, 31 Aug 2026 21:27:00 -0400 Subject: [PATCH 014/190] =?UTF-8?q?=F0=9F=93=96=20changelog:=20backend=20s?= =?UTF-8?q?moke,=20run-record=20telemetry,=20and=20the=20relay=20fixes=20t?= =?UTF-8?q?hey=20found?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Jo76R9XXJ2iq6YcmVUiX2u Signed-off-by: Douglas Baggett --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8995ea826..2d93df2c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ Hive did not historically maintain a complete changelog. This file starts a prag ### Added - Google Antigravity (`agy`) is now selectable as an agent method on the dashboard instead of being launchable only through hand-edited configuration. Its model dropdown is backend-specific rather than inheriting the unrelated Copilot catalog: the first paint uses the 11 model IDs reported by `agy models` in Antigravity CLI 1.1.18 (Gemini 3.7/3.6 Flash effort variants, Gemini 3.1 Pro, Claude Sonnet/Opus 4.6, and GPT-OSS 120B), then `/api/config/backends` replaces that floor with the signed-in account's live `agy models` inventory. The probe runs the vendor CLI against the same shared `.gemini` state as hive agents, without reading or logging OAuth material; a missing binary, signed-out account, timeout, or changed/empty output remains a non-fatal, explicitly marked static fallback. The existing launcher contract is unchanged: selecting one of these model IDs still supplies both `--model` and agy's required `--effort low`. +- A live backend smoke for the contributor CLI integration. The vendor coding-agent CLIs hive drives (claude, codex, …) are a seam hive does not control, and every existing test pins it against captured fixtures — so a vendor CLI update ships green here and breaks contributors in production. `bin/test_backend_smoke.sh` (also `just backend-smoke`) now drives the REAL relay against a fake WebSocket hub and, where a credential exists, the real backend CLI on a one-line task, asserting the wire contract end to end: task acceptance, the `HIVE_VERDICT` sentinel, `completion_signal: verdict` rather than the chrome-idle fallback, and the `task_failed` failure-kind shape. A keyless subset (backend-list/Dockerfile-pin drift checks plus stub-CLI wire scenarios) runs merge-gating in v2-ci; the live scenarios run on a 6-hour schedule in the new `backend-smoke.yml` — a matrix of {claude, codex} × {latest vendor release, pinned contributor image} with per-lane deduplicated failure issues, so "vendor drift incoming" and "contributors broken today" never blur into one thread. It is the repo's only workflow holding model credentials (metered API keys, or base64 subscription-login blobs for projects funding it from a Claude Pro/Max or ChatGPT account), scoped per matrix arm, running the cheapest model tiers on a no-file-access one-liner. Its first full run caught the three relay bugs in this release's Fixed section — the exact failure class it exists for. `src/Dockerfile.contributor`'s codex pin is now an `ARG` asserted equal to `src/Dockerfile`'s, closing an unguarded drift the suite's first keyless run found. +- Durable per-run contributor task telemetry with a scenario ratchet. Every accepted `task_complete`/`task_failed` already carried a normalized closed vocabulary — `completion_signal`, the failure kind, the `no_work_needed` verdict — but none of it survived anywhere aggregable: the log line rotates away, `activity.json` caps at 50 entries and drops reason/kind, and nothing recorded durations at all. The hub now appends one JSONL record per terminal task report to `/data/contributors/task_runs.jsonl` (0600, 10 MiB cap, one rotation) carrying those normalized fields, the task's real wall-clock duration, and a derived `scenario` from a closed, stable set: `verdict_complete`, `idle_complete` (the ratchet metric — sentinel non-compliance), `headless_complete`, `env_failure`, `task_failure`, `unspecified_failure`. `GET /api/contribute/run-stats` serves per-backend scenario counts, the chrome-idle share, and duration percentiles over a trailing window (public read-only like the sibling contribute reads — aggregates only, no usernames or reasons). DECLARE only, per the `contribute_protocol.go` boundary: nothing routes, cooldowns, or gates on any of it, and a telemetry write failure never fails a task. - `AGENTS.md` repo instructions now actually reach agent kicks ([#5227](https://github.com/kubestellar/hive/issues/5227)). Hive shipped a complete, tested `AGENTS.md` parser (`pkg/agentsmd`) and a kick call site that prepends its output, but the one function supplying that call site a repo checkout path, `Scheduler.agentsRepoRoot()`, returned `""` unconditionally — so the guard could never fire and the parser was never invoked for any repo. A test even pinned the empty return, enforcing the feature staying dead. The root is now resolved per repo from a new optional `project.checkouts_dir`: each repo is looked up at `/`, so a multi-repo hive gets the primary repo's own `AGENTS.md` and never a different repo's. `policies.local_dir` — a real checkout root Hive already reads policy files from — is used as a second source, but only when `policies.repo` names the repo being asked about, so a config repo's instructions never leak into work on an unrelated repo. **Unset is the default and preserves the previous behavior exactly:** Hive agents work over the API and keep no clones, so a hive that configures no checkout has no root, and injection stays the no-op it was. Everything stays fail-open — an absent directory, a missing `AGENTS.md`, or a blank one yields no injection and never fails a kick — and the scheduler now logs at debug which root came up empty, so a wired-but-empty repo is finally distinguishable from an unconfigured hive. Closest-wins nested `AGENTS.md` (`agentsmd.ParseNearest`) remains deferred: it needs file-level targeting, which the kick path does not have. [`src/docs/agents-md.md`](src/docs/agents-md.md) is rewritten from "not wired into kicks" to how to turn it on, and the cross-references in `README.md`, `agent-configuration.md` and `skills.md` are corrected. - A companion page [`src/docs/forge-app-setup.md`](src/docs/forge-app-setup.md) covering the non-GitHub forges, because `github-app-setup.md` is scoped to GitHub.com and GHE while its own terminology note — echoed by `getting-started.md` and `troubleshooting.md` — told GitLab, Gitea, and Forgejo operators that the Forge App was "the equivalent host app" on their platform and pointed them at a GitHub-only guide. That framing was wrong in a way an operator could only discover after attempting an install. The new page states plainly that a hive **cannot run against GitLab, Gitea, or Forgejo today**: `src/pkg/forge` ships tested GitHub/GitLab/Gitea adapters, but nothing imports the package outside its own tests, so `project.forge: gitlab` changes only the dashboard's Platform tile. It documents why — agents reach their forge through the `gh` CLI wrapper and the `hive-open-pr`/`hive-open-issue` request-file path, not through the abstraction, and the GitHub MCP write tools are explicitly denied — plus the ceiling on the abstraction itself (no `CreateIssue`, no `CreatePR`, and `Merge` left an explicit interface TODO). It also documents the `gitlab:`/`gitea:` config blocks that genuinely parse, as **top-level** siblings of `github:` rather than nested inside it, and disambiguates `project.forge` (forge family) from `github.forge` (which GitHub instance a hive's App lives on) — two unrelated settings sharing a word, where confusing them breaks a working hive. The three misleading cross-references are corrected in place ([#5284](https://github.com/kubestellar/hive/issues/5284)). @@ -51,6 +53,8 @@ Hive did not historically maintain a complete changelog. This file starts a prag Alongside it, a new `claude.HasUsableToken` (a live access token, **or** an expired one whose refresh grant is still good) replaces `HasValidToken` at the five sites that were asking "can this credential still put an agent to work?" rather than "is this token live right now?". `HasValidToken` reports an expired token as no token at all, which made a routine daily expiry indistinguishable from a real logout: the dashboard painted the 🔑 badge, the credential watchdog logged the durable credential "unusable" and prescribed an operator device-flow login, the agent watchdog raised `Agent "…" needs re-authentication (PaneShowsLogin)`, and the **token-triggered restart heal stood down** — the heal built for exactly "login prompt on screen while the credential is valid" ([#4596](https://github.com/kubestellar/hive/issues/4596)/[#4606](https://github.com/kubestellar/hive/issues/4606)) was disabled by the most common reason its pane appears. The watchdog now reports `LoginPromptWithUsableCredential` (Authenticated=Unknown, no alert) when a restart can recover the pane. **The genuinely-logged-out path is untouched:** with no refresh grant, or one past its own expiry, every alert, badge and prescription behaves exactly as before — this can only suppress a page it can justify from evidence on disk, never invent one. Hive still performs no refresh of its own. A refresh rotates the grant and instantly revokes the access token every *other* live session is holding (observed verbatim as `401 OAuth access token has been revoked` on a sibling agent), which is the race [#5171](https://github.com/kubestellar/hive/pull/5171) declined to introduce; redemption stays in the CLI's own start-up path. `OAuthTokens` also gained the `refreshTokenExpiresAt` field it had been silently dropping on any rewrite — the only field that distinguishes a refreshable credential from a spent one. +- **Interactive claude completions now end on the agent's own verdict instead of silently degrading to the chrome-idle fallback.** The `HIVE_VERDICT` scanner ([#5376](https://github.com/kubestellar/hive/issues/5376)) tolerated a leading bullet only in codex's spelling (`•`, U+2022); Claude Code renders its assistant lines with a filled circle (`●`, U+25CF), so a claude agent that printed the sentinel exactly as instructed was never heard — every interactive claude completion waited out three idle-chrome checks and reported `completion_signal: chrome_idle`, leaving the fleet one vendor chrome restyle away from the pre-#5376 failure class the sentinel was built to end. Found by the new backend smoke's first live run: the pane capture showed the verdict on screen while the relay logged "no HIVE_VERDICT yet". The scanner now accepts both glyphs; same anchoring, same prompt-echo guard, one parser as before. +- **Headless codex runs work.** Two independent defects, both found live by the backend smoke: `codex exec` announces "Reading additional input from stdin…" and blocks on stdin-EOF even with the prompt passed in argv — and the relay's `execFile` never closed the child's stdin pipe, so a headless codex task produced zero output and hung until the task timeout killed it (reported as an environment failure with an empty tail). The relay now closes every one-shot child's stdin — a one-shot invocation has no interactive input coming, whatever the backend. Separately, `codex exec` refuses outright to run in a cwd that is not a git repository, and the task workspace root is exactly that (the agent clones *into* it as its first act); the codex headless argv now passes `--skip-git-repo-check`. - **The entrypoint behavioural tests now actually run, and a skipped one fails the lane** ([#5380](https://github.com/kubestellar/hive/issues/5380)). `test_entrypoint_runtime_config.sh` and `test_entrypoint_data_ownership.sh` each end in the assertion that is the whole reason the file exists: create a root-owned file, then really `open()` it as the uid the hive process drops to. That is the check mode-inspection cannot make — [#5360](https://github.com/kubestellar/hive/issues/5360) shipped green behind a mode-only assertion and took four merges to diagnose. Both blocks need root **and** a `dev` account, and both suites ran only on `ubuntu-latest`, where the runner user is uid 1001 but named `runner` — so `id -u dev` failed and **both blocks skipped on every PR**. They skipped loudly rather than faking a pass, which was the right design, but nothing acted on the skip: the strongest assertions in either file had never executed in CI. They now also run inside the arm64 podman lane's container, which already builds the PR's own image ([#5370](https://github.com/kubestellar/hive/issues/5370)) and whose image has root and `dev` at uid 1001, so the assertions execute against the code under review rather than code already merged. In that lane `HIVE_TEST_REQUIRE_BEHAVIOURAL=1` makes a skip a **failure**: where root and `dev` are known to exist, a skip cannot mean "unsuitable environment", it means a precondition changed and the test quietly stopped testing — which is what kept this class of gap alive. Nothing the suites assert was weakened, and the bare-runner path is unchanged: both still skip loudly and exit 0 on an unprivileged runner or a laptop, so they stay runnable anywhere. From 90e5ca6358afd72cb5bb7f3c179b6ba09e8118ea Mon Sep 17 00:00:00 2001 From: Douglas Baggett Date: Mon, 31 Aug 2026 21:35:08 -0400 Subject: [PATCH 015/190] =?UTF-8?q?=F0=9F=93=96=20docs:=20backend-smoke=20?= =?UTF-8?q?reference=20page?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The smoke suite, its scheduled workflow, and the run telemetry were documented only in file headers and the changelog — nothing under src/docs/, which is what syncs to kubestellar.io and where an operator or maintainer actually looks. The new page covers the questions the code comments answer piecemeal: who runs what (the project's CI, centrally — hive operators run nothing and fund nothing), the latest/pinned lane distinction and what a red in each means, the per-lane deduplicated issue-filing flow with its evidence artifact, both credential forms with their trade-offs and the activation checklist, and the task_runs.jsonl scenario vocabulary with the idle_complete ratchet behind /api/contribute/run-stats. Indexed in src/docs/README.md beside the ClankeR relay page; check-docs-links.py passes. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Jo76R9XXJ2iq6YcmVUiX2u Signed-off-by: Douglas Baggett --- src/docs/README.md | 1 + src/docs/backend-smoke.md | 155 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+) create mode 100644 src/docs/backend-smoke.md diff --git a/src/docs/README.md b/src/docs/README.md index f552b45ba..2e025ad9c 100644 --- a/src/docs/README.md +++ b/src/docs/README.md @@ -52,6 +52,7 @@ Start with [Architecture](architecture.md) for the system overview, then use the - [Getting started as a first-time contributor](https://github.com/kubestellar/hive/blob/v4/docs/getting-started-contributing.md) — the end-to-end path for a first code or documentation contribution, tying the reference docs together and answering the Hive-specific questions they don't. - [Local development](https://github.com/kubestellar/hive/blob/v4/docs/development.md) — the local workflow for contributing to the Go codebase on `v4`: prerequisites, build, and test loop. - [ClankeR contributor relay](contributor-relay.md) — local contributor setup, multi-hub subscriptions, moving a relay to another machine, and role requests. +- [Backend smoke](backend-smoke.md) — the live canary for the contributor CLI integration: who runs it (the project's CI, centrally — not hive operators), the latest/pinned lanes and what a red in each means, how failures become deduplicated issues with evidence attached, the API-key vs subscription-login credential options, and the per-hive run telemetry behind `/api/contribute/run-stats` with its scenario ratchet. - [Contributor trust tiers and delegated agent roles](https://github.com/kubestellar/hive/blob/v4/src/docs/contributor-trust-and-roles.md) — newcomer/contributor/trusted/merger/advisor semantics, **Acting as**, grants, and delegatable roles. - [Credly badges](https://github.com/kubestellar/hive/blob/v4/src/docs/credly-badges.md) — planned integration design; currently a placeholder mapping only. diff --git a/src/docs/backend-smoke.md b/src/docs/backend-smoke.md new file mode 100644 index 000000000..f0d47c481 --- /dev/null +++ b/src/docs/backend-smoke.md @@ -0,0 +1,155 @@ +# Backend smoke — the live canary for contributor CLI integration + +Hive runs contributor tasks by driving vendor coding-agent CLIs — Claude Code, +Codex, and the rest of the [backend list](https://github.com/kubestellar/hive/blob/v4/docs/backend-setup.md) — +through the ClankeR relay. That seam is the one integration surface hive does +not control: vendors ship CLI updates on their own schedule, and what breaks +is readiness detection, completion detection, credential layout, and one-shot +invocation behavior. Every other test in this repo pins that seam against +captured fixtures, so a vendor change ships green in CI and fails contributors +in production. + +The backend smoke is the live complement: +[`bin/test_backend_smoke.sh`](https://github.com/kubestellar/hive/blob/v4/bin/test_backend_smoke.sh) +drives the **real relay** against a fake in-process hub and — where a +credential exists — the **real backend CLI** on a one-line task, asserting the +machine-checkable contract end to end: task acceptance, the `HIVE_VERDICT` +sentinel, `completion_signal: verdict` rather than the chrome-idle fallback, +and the `task_failed` wire shape. + +Its first full run caught three live relay bugs (the claude `●` verdict glyph +the parser did not accept, a headless-codex stdin hang, and codex's non-git +workspace refusal) — the exact failure class it exists for. See the +[Changelog](https://github.com/kubestellar/hive/blob/v4/CHANGELOG.md) entries. + +## Who runs what + +| Piece | Runs where | Who acts on it | +| --- | --- | --- | +| Scheduled live smoke ([`backend-smoke.yml`](https://github.com/kubestellar/hive/blob/v4/.github/workflows/backend-smoke.yml)) | kubestellar/hive's own GitHub Actions, every 6h — gated to this repo, so forks and downstream deployments never run it | hive maintainers, via the issues it files | +| Keyless subset | v2-ci, merge-gating on every PR | the PR author, like any red check | +| `just backend-smoke` | any laptop or hive host, opt-in, with that machine's own credentials | whoever ran it | +| Run telemetry (below) | **every hive hub, passively** — no setup, no model spend | the hive's operator; fleet-level aggregation is a planned follow-up | + +Nobody operating a hive has to run or fund the smoke. It answers one global +question — "is hive's integration with the vendor CLIs still sound" — so it +runs centrally, once. + +## What the suite tests + +Three tiers; each degrades cleanly when its prerequisites are missing: + +- **Drift checks (keyless, deterministic).** The relay's `HEADLESS_BACKENDS` + table must agree with `KNOWN_BACKENDS` in `config/backends.conf` (previously + kept in sync by comment only), and the claude/codex version pins must match + between `src/Dockerfile` and `src/Dockerfile.contributor`. +- **Stub wire-contract scenarios (keyless).** A stub CLI binary on `PATH` + drives the full relay↔hub loop — handshake, `task_assign`, verdict parsing + off one-shot output, the failure shape — with zero API spend. This tier plus + the drift checks is what v2-ci runs. +- **Live per-backend scenarios (needs a CLI + credential).** Per backend: the + `detect_cli` health probe, a headless end-to-end run (`claude -p` / + `codex exec` on a one-line no-file-access task, asserting the + `HIVE_VERDICT: no_work_needed` reply on the wire), and an interactive + end-to-end run (the real CLI in tmux, the relay scraping the pane, asserting + `completion_signal == "verdict"` — a `chrome_idle` completion is a FAILURE + here, because it means the sentinel contract is broken and only the fallback + saved the task). + +Knobs (see the script header for the full list): `HIVE_SMOKE_BACKENDS` +(default `claude codex`), `HIVE_SMOKE_MODEL_CLAUDE` / `HIVE_SMOKE_MODEL_CODEX` +(cheapest tiers by default — the plumbing is under test, not the model), and +`HIVE_TEST_REQUIRE_BACKEND_SMOKE=1`, which turns every skip into a failure. +That inversion is what keeps the scheduled lane honest: on a runner where +credentials are *supposed* to exist, "skipped for lack of credentials" is a +failure, not a green. + +## The two lanes, and what a red means + +The scheduled workflow runs a matrix of backends × two lanes, and the +distinction is the point: + +- **latest** — installs the vendor's current CLI release on the runner. + Red means **vendor drift is incoming**: shipped contributor images still + work, but the next pin bump will break. Fix the integration before bumping. +- **pinned** — runs the suite inside + `ghcr.io/kubestellar/hive-contributor:latest`, against the CLI versions + contributors actually get. Red means **contributors are likely broken right + now**. + +## How failures are reported + +- A red **scheduled** run files one issue **per failing lane**, titled + `[Backend smoke: latest] failure on ` or + `[Backend smoke: pinned] …`, labeled `backend-smoke, ci-failure, kind/bug`. + The body states the lane's meaning in operator terms and links the run. +- **Dedupe is per lane**: if an open `backend-smoke` issue with that lane's + title prefix exists, the run comments on it instead of filing another — a + multi-day vendor outage is one issue with a comment trail, not a pile. +- **Only `schedule` events file.** A red `workflow_dispatch` triage run never + touches the tracker. +- **Evidence travels with the failure.** On any failed scenario the suite + prints the relay log tail, the fake hub's wire-message tail, and (for + interactive scenarios) a tmux pane capture into the workflow log, and the + full output is uploaded as a `backend-smoke--` artifact. The + linked run answers "what did the CLI actually print" without a repro. + +## Credentials + +The workflow holds the repo's only model credentials, scoped per matrix arm +(claude arms never see codex credentials and vice versa). Two forms per +vendor; either is sufficient: + +| Secret | Form | Trade-off | +| --- | --- | --- | +| `ANTHROPIC_API_KEY` / `OPENAI_API_KEY` | metered API key | preferred for an unattended schedule: never expires, own rate limits; costs ~32 cheapest-tier one-line calls/day | +| `HIVE_SMOKE_CLAUDE_CREDENTIALS_B64` / `HIVE_SMOKE_CODEX_AUTH_B64` | `base64 -w0 ~/.claude/.credentials.json` from a Claude Pro/Max login, or `base64 -w0 ~/.codex/auth.json` from a ChatGPT sign-in | flat-rate, but the OAuth refresh chain rotates so the stored blob **goes stale** and must be re-captured from a fresh login (a stale one reds the live scenarios), and the smoke shares the account's rate limits with its human | + +Local runs need none of this: the suite falls back to copying the machine's +own logged-in credential into a throwaway `HOME`, so a laptop with a +subscription login runs the full live arms with zero setup. + +**Activation checklist** (maintainers, once): add one credential secret per +backend, then run one `workflow_dispatch` per lane and confirm both go green. + +## Run telemetry: `task_runs.jsonl` and `/api/contribute/run-stats` + +The smoke asks its questions synthetically; every hive hub also records the +answers for its **real** contributor traffic. Each accepted +`task_complete`/`task_failed` appends one JSONL record to +`/data/contributors/task_runs.jsonl` (0600, 10 MiB cap with a single `.1` +rotation) carrying the hub-normalized fields — `completion_signal`, the +failure kind, the verdict — plus the task's wall-clock duration and a derived +`scenario` from a closed vocabulary: + +| Scenario | Meaning | +| --- | --- | +| `verdict_complete` | the agent ended the task with its own `HIVE_VERDICT` sentinel — the contract working | +| `idle_complete` | completed only via the chrome-idle fallback — the sentinel contract is NOT honored for this backend. **The primary ratchet metric.** | +| `headless_complete` | completed with no completion signal on the wire: the headless one-shot path (whose completion is the exit code), or a pre-#5376 relay | +| `env_failure` | the client's runtime could not run the work — the broken-integration class the smoke hunts | +| `task_failure` | the work was attempted and failed on its merits | +| `unspecified_failure` | a failure with no usable kind (older relay or unrecognized value) | + +`GET /api/contribute/run-stats?days=N` (default 7; public read-only, aggregate +counts only — no usernames, reasons, or tokens) serves per-backend scenario +counts, the chrome-idle share, and duration p50/p95. The intended use is a +ratchet: watch `idle_complete` share and `env_failure` counts per backend +trend toward zero, and treat a jump as a regression to root-cause. + +This is DECLARE-only telemetry, per the boundary +`src/pkg/dashboard/contribute_protocol.go` documents: nothing routes, +cooldowns, or gates on any of it, and a telemetry write failure never fails a +task. + +## Running it locally + +```sh +just backend-smoke # both default backends +just backend-smoke claude # one backend +bash bin/test_backend_smoke.sh # same thing, no just required +``` + +Keyless machines run the drift and wire-contract tiers and print loud `SKIP` +lines for the live ones. `HIVE_TEST_REQUIRE_BACKEND_SMOKE=1` reproduces the +scheduled lane's strictness exactly. From fcf7d0da78919535645c97294aab3ff09b7e764a Mon Sep 17 00:00:00 2001 From: Andy Anderson Date: Tue, 1 Sep 2026 10:04:40 -0400 Subject: [PATCH 016/190] =?UTF-8?q?=F0=9F=93=9D=20docs:=20operator=20diagn?= =?UTF-8?q?ostics=20for=20unpublished=20agent=20work=20and=20relay=20redac?= =?UTF-8?q?tion=20scope=20(#5474)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 📝 docs: operator diagnostics for unpublished agent work and relay redaction scope Three adjacent operator-facing gaps, all in src/docs/. troubleshooting.md gains "An agent session completes but no branch or PR appears" — the symptom is invisible in the fleet view because the agent genuinely is healthy. Two causes produce it and the section separates them by timing signature: the credential helper unreachable from the agent's UID (#5343/#5352, fails the FIRST push of every task on the whole hive) versus a silent mid-task token re-mint failure (#5447, fails ~an hour in after earlier pushes in the same task succeeded). The entrypoint's own boot-time probe is the fastest discriminator, so the section leads with reading it back. hive-open-pr.md gains "Diagnosing a PR request that never opens", documenting the result-file shapes, the quarantine suffixes, the 30s→15m backoff with its 24h give-up horizon, and the three outcomes the watcher now distinguishes when compare returns 404: repo invisible to the App, head ref genuinely unpushed (reported as a push-authentication failure, and retryable — pushing the branch makes the queued request valid), and a 404 that was really about the base. security.md corrected for scope. It stated that Hive wraps slog with pkg/logscrub without noting that the contributor relay is a separate Node process which never loads it and redacts independently via redactTokens(). A reader concluded relay logs were covered by pkg/logscrub; they are covered by something else with a different pattern list. Both layers are now described with an explicit divergence table. Two corrections to the previously documented pattern list, which was stale: logscrub also covers ghu_/ghr_, HIVE-CANARY, AWS AKIA/ASIA, Bearer values and PEM private-key blocks. And the relay is the NARROWER layer, not the broader one — it matches GitHub token prefixes only, missing JWTs, canaries, AWS keys, Bearer headers and private keys, and requires 36+ trailing characters where Go requires 10+. Fixes #5468 Fixes #5469 Fixes #5470 Signed-off-by: Andrew Anderson * 📝 docs: fix miscount in security.md redaction differences list Signed-off-by: Andrew Anderson --------- Signed-off-by: Andrew Anderson --- src/docs/hive-open-pr.md | 93 +++++++++++++++++++++++++++++++++++++ src/docs/security.md | 42 ++++++++++++++++- src/docs/troubleshooting.md | 84 +++++++++++++++++++++++++++++++++ 3 files changed, 217 insertions(+), 2 deletions(-) diff --git a/src/docs/hive-open-pr.md b/src/docs/hive-open-pr.md index 5ccad7d62..858f9595d 100644 --- a/src/docs/hive-open-pr.md +++ b/src/docs/hive-open-pr.md @@ -63,6 +63,99 @@ comment on it will fail. To confirm, poll the `.result.json` written next to the request file, or simply look for the PR. +## Diagnosing a PR request that never opens + +When the PR does not appear, the request file's `.result.json` is the record of +why. It sits next to the request in `/var/run/hive-metrics/pr-requests`, named +`.result.json`, and on failure carries `"ok": false` with an `error` +string. + +The request file itself is also renamed once it stops being retried, and the +suffix tells you which kind of failure it was: + +| Suffix | Meaning | Retried? | +| --- | --- | --- | +| *(none)* | still queued | yes, with backoff | +| `.failed` | transient failure that never recovered within the give-up horizon | no longer | +| `.rejected` | a policy gate refused the content — the request or branch must change | no | +| `.denied` | authorization refused (ACMM write-gate, forge-resistance) | no | + +A transient failure backs off exponentially from 30s to a 15-minute ceiling and +is quarantined as `.failed` only after **24 hours** without success. So a +freshly failing request is normal-looking for a while: read `.result.json` +rather than waiting for a rename. + +### The 404 that means "your push failed" + +Every gate on the PR-open path begins by comparing `base...head`. If the +agent's branch was never pushed, GitHub answers 404 — and the message used to +read as though the *branch* were the problem, which sends an operator to +investigate branch creation. That is the wrong place: the branch is missing +because the **push** failed. + +The watcher now investigates a 404 before reporting it ([#5343](https://github.com/kubestellar/hive/issues/5343), +fixed in [#5352](https://github.com/kubestellar/hive/pull/5352)). It probes the +**repository** first, then the **head ref**, and reports one of three distinct +outcomes. Only a definitive 404 is investigated — a 403, a rate limit, or a 5xx +keeps its own identity so the existing retry and rate-limit handling still +recognise it. + +**1. The App cannot see the repository.** The repo probe 404s: + +> cannot open a PR on `/`: this hive's GitHub App cannot see that repository (404). Check that the App is installed on it and that the installation grants contents+pull_requests access — this is NOT a problem with branch `` + +The branch is not implicated at all. Fix the App installation — see +[GitHub App setup](github-app-setup.md). + +**2. The repository is visible but the head ref is genuinely absent.** This is +the push-authentication case, and the error says so: + +> branch `` was never pushed to `/` — the commits exist only in the agent's working copy. This is almost always a PUSH AUTHENTICATION failure, not a branch-creation problem + +It names the two causes to check, in order: + +1. **The git credential helper is not reachable from the agent's UID.** + `su -s /bin/sh hive- -c 'git config --get-regexp credential'` must + list `/usr/local/bin/git-credential-hive.sh`. It is wired system-wide in + `/etc/gitconfig` precisely because agents do not share the dev user's + `$HOME`. +2. **The agent's scoped token file is absent or unreadable by that UID.** The + path is in `$HIVE_AGENT_TOKEN_CACHE`. Check readability only — never print + its contents. + +This stays a **retryable** error rather than a policy rejection: once the agent +pushes the branch, the same request becomes valid and succeeds on retry. That +is exactly what the bounded retry is for, so fix the credential problem and let +the queued request land — you do not need to re-issue it. + +**3. The head ref exists, so the 404 was about something else.** Usually the +base: + +> GitHub returned 404 although branch `` exists — check that the base branch `` exists on the remote + +Note the watcher deliberately does **not** try to read the agent's git state or +test the credential helper itself: it runs in the hive process, not in the +agent's UID, so any such check would answer a different question than the one +that failed. It names the causes; you verify them from the agent's UID. + +### The log line to grep for + +Case 2 is also logged at ERROR on its own line, because it is work an agent +already completed and cannot publish — and it is invisible in the fleet view, +since the agent's session ended healthy: + +```sh +kubectl -n hive logs deploy/hive | grep 'pr-request watcher' +``` + +Look for `head branch is not on the remote — the agent's push did not +authenticate`, which carries `repo`, `head`, `agent`, and the full `diagnosis`. + +For the broader symptom — an agent that completed a session with no branch and +no PR, and how to tell a credential-helper failure from a mid-task token +refresh failure — see +[Troubleshooting: an agent session completes but no branch or PR appears](troubleshooting.md#an-agent-session-completes-but-no-branch-or-pr-appears). + ## Where things live | Path | What | diff --git a/src/docs/security.md b/src/docs/security.md index 7aa47b2ac..7ecbb726f 100644 --- a/src/docs/security.md +++ b/src/docs/security.md @@ -4,6 +4,8 @@ Hive wraps its structured `slog` handler with `pkg/logscrub`. The wrapper redacts recognized token-like strings before log records reach the underlying handler. +**This covers the Go process only.** The contributor relay is a separate Node process and does not use `pkg/logscrub` at all — it carries its own, independently written redaction. The two are described separately below, because they do not cover the same things. See [Two redaction layers, not one](#two-redaction-layers-not-one). + ## What is scrubbed The current redaction pattern replaces matches with `[REDACTED]` in: @@ -15,16 +17,52 @@ The current redaction pattern replaces matches with `[REDACTED]` in: Recognized patterns are: -- GitHub token prefixes `ghs_`, `ghp_`, `gho_`, and `github_pat_` followed by at least ten token characters. +- GitHub token prefixes `ghs_`, `ghp_`, `gho_`, `ghu_`, `ghr_`, and `github_pat_` followed by at least ten token characters. - JWT-like strings beginning with `eyJ` and containing three base64url segments. +- Hive canary values (`HIVE-CANARY-` followed by 48 hex characters). +- AWS access key IDs (`AKIA`/`ASIA` followed by 16 uppercase alphanumerics). +- `Bearer` authorization values of 16 characters or more. +- PEM private-key blocks — RSA, EC, OpenSSH, DSA, encrypted, and PGP private-key blocks — redacted whole, including multi-line bodies. + +The GitHub and JWT shapes live in the exported `logscrub.TokenPattern`, which other packages (for example `pkg/ioscan`) reuse rather than duplicating; keep that the single source of truth. The cost endpoint also redacts the configured gateway API key from native-cost probe errors before returning the error text. +## Two redaction layers, not one + +`pkg/logscrub` protects the **Go process**. The contributor relay (`bin/contributor-relay.sh`) is a **separate Node process** that never loads it, and redacts on its own with a `redactTokens()` function. Both exist and both work — but they are separate implementations with separate pattern lists, and reading only the section above would leave you assuming a coverage the relay does not have. + +The relay applies `redactTokens()` to agent output before it leaves the host: to the captured tmux tail, and to the combined stdout/stderr tail of a headless task. See [Contributor relay](contributor-relay.md). + +### Where they differ + +| Shape | `pkg/logscrub` (Go) | relay `redactTokens()` (Node) | +| --- | :---: | :---: | +| `ghs_` `ghp_` `gho_` `ghu_` `ghr_` | yes | yes | +| `github_pat_` | yes | yes | +| JWTs (`eyJ…` triples) | yes | **no** | +| `HIVE-CANARY-…` | yes | **no** | +| AWS `AKIA`/`ASIA` keys | yes | **no** | +| `Bearer ` | yes | **no** | +| PEM private-key blocks | yes | **no** | +| Backend provider credentials | no | Pi backend only | + +Three further differences worth knowing: + +- **Minimum match length.** The Go patterns require 10 or more characters after the prefix; the relay requires 36 or more. A short or truncated token-shaped string is redacted by Go and passed through by the relay. The relay's bound is deliberately `{36,}` rather than `{36}` so a *longer* token is not redacted only in its first 36 characters, leaking its tail ([#4267](https://github.com/kubestellar/hive/issues/4267)). +- **Character class.** Go accepts `[A-Za-z0-9_]` after every prefix. The relay accepts only `[A-Za-z0-9]` for the `gh*_` prefixes (underscores allowed for `github_pat_` alone). +- **Placeholder text.** Go writes `[REDACTED]`; the relay writes `_***REDACTED***`, keeping the prefix visible. Alerting that greps for one will not match the other. + +When the relay's backend is Pi, `redactTokens()` additionally strips the configured provider credential values by literal substring match — a mechanism neither layer applies to GitHub tokens. + +**The practical reading:** GitHub token material is covered on both paths. Everything else in the Go list — JWTs, canaries, AWS keys, `Bearer` headers, private keys — is redacted in the hive's own logs and **not** in relay-forwarded agent output. If an agent's terminal prints a JWT or a private key, the relay will forward it. + ## Limits - Scrubbing is pattern-based, not a general secret scanner. A secret with another shape can still appear if code logs it directly. - Non-string slog values are passed through unchanged unless they are inside a group containing string attributes. -- Redaction happens in the Hive logging path. Data written by external tools, agent CLIs, terminal transcripts, or third-party proxies is not automatically covered unless it flows through Hive's scrubbed logger. +- `pkg/logscrub` redaction happens in the Hive logging path. Data written by external tools, agent CLIs, or third-party proxies is not automatically covered unless it flows through Hive's scrubbed logger. Agent terminal output forwarded by the contributor relay is a partial exception — it is redacted, but by the relay's own narrower pattern list, not by `pkg/logscrub`. +- The two layers can drift. They are separate implementations with no shared source of truth, so a pattern added to one does not appear in the other; the table above is accurate as of writing and worth re-checking against both implementations before relying on it. - False positives are replaced with `[REDACTED]`; there is no runtime allow-list or custom pattern configuration today. ## Operator checks diff --git a/src/docs/troubleshooting.md b/src/docs/troubleshooting.md index ca0f0d542..cdedd4ec4 100644 --- a/src/docs/troubleshooting.md +++ b/src/docs/troubleshooting.md @@ -129,6 +129,90 @@ Liveness is judged by the governor's in-process health check, so an agent that k 1. **Read the work counts, not just liveness.** If an agent reports "Issues triaged: 0" cycle after cycle in the logs, that is the signal — `kubectl -n hive logs deploy/hive | grep ` or attach to the session. 2. **Cross-check an external surface.** Confirm the effect the agent is supposed to produce (a GitHub API query for the PRs/issues it claims to have handled) rather than trusting its self-reported state. +## An agent session completes but no branch or PR appears + +The agent ran, the session ended cleanly, the fleet view shows it healthy — and there is no PR and no branch on the remote. Often the agent's own summary says so plainly, in words like "branch committed locally but push failed due to git authentication issue." + +This is not the agent deciding no work was needed. The work was done; it could not be published. The fleet view cannot tell you which, because from the governor's point of view the session *is* healthy — the agent hit an auth error, correctly refused to manipulate git credentials, and wrote an honest summary. See [kubestellar/hive#5343](https://github.com/kubestellar/hive/issues/5343). + +There are two distinct causes with the same symptom, and they are distinguishable. + +### First: confirm the work exists and is unpublished + +```sh +# Does the branch exist on the remote at all? +gh api "repos///git/ref/heads/" 2>&1 | head -3 + +# Did the agent commit it locally? (per-agent HOME, not the dev user's) +ls -d /data/home/agents//* 2>/dev/null +``` + +A 404 from the first command plus commits in the agent's working copy is this scenario. If the branch *is* on the remote, the problem is downstream — go to [`hive-open-pr`](hive-open-pr.md#diagnosing-a-pr-request-that-never-opens) instead. + +### Cause 1 — the credential helper is not reachable from the agent's UID + +The helper is invoked per-UID, and agents do **not** share the dev user's `$HOME`: each per-agent UID runs with its own `$HOME` under `/data/home/agents/`, which has no `.gitconfig`. `git config --global` writes to the *caller's* `$HOME`, so wiring the helper that way makes it invisible to every agent. The helper is therefore wired **system-wide in `/etc/gitconfig`**, written from the entrypoint's root phase — see [#5343](https://github.com/kubestellar/hive/issues/5343) for the original defect and [#5352](https://github.com/kubestellar/hive/pull/5352) for the fix. + +Check the layer that actually matters, from a process with no per-user config — this is the same probe the entrypoint runs at boot: + +```sh +# Inside the hive container. Empty output = the helper is invisible to agents. +HOME=/nonexistent XDG_CONFIG_HOME=/nonexistent \ + git config --get-regexp '^credential\.' | grep git-credential-hive.sh + +# Or ask as the agent UID directly: +su -s /bin/sh hive- -c 'git config --get-regexp credential' + +# The file that supplies it — must exist and be world-readable (0644). +ls -l /etc/gitconfig +``` + +Each should list `/usr/local/bin/git-credential-hive.sh`. The boot log records the same verdict, so you can also just read it back: + +```sh +kubectl -n hive logs deploy/hive | grep 'git credential helper' +``` + +- `git credential helper VERIFIED reachable without a per-user .gitconfig` — this cause is ruled out. Go to cause 2. +- `WARN: git credential helper is NOT reachable ...` — this is your cause. Every agent on this hive will commit branches it cannot push. Restart the hive so the entrypoint's root phase rewrites `/etc/gitconfig`; if that phase never ran (a boot that could not become root), only the dev user's global config exists and no agent will ever push. + +Also confirm the agent's own scoped token is present and readable by its UID — the helper needs it: + +```sh +su -s /bin/sh hive- -c 'test -r "$HIVE_AGENT_TOKEN_CACHE" && echo readable || echo MISSING' +``` + +Never print the file's contents. + +### Cause 2 — the credential went stale mid-task (silent refresh failure) + +Contributor-relay tasks are pushed with a scoped token the hub re-mints periodically, a few minutes before its TTL expires. When a re-mint **fails**, the hub logs a warning and keeps the old token; the relay is told nothing. See [kubestellar/hive#5447](https://github.com/kubestellar/hive/issues/5447). + +The signature is different from cause 1, and it is a timing signature: + +| | Cause 1 (helper unreachable) | Cause 2 (refresh failed) | +| --- | --- | --- | +| Which agents | **all** agents on the hive | usually one long-running task | +| When the push fails | the **first** push of any task | roughly an hour in, after earlier pushes in the *same* task succeeded | +| Boot-log probe | `WARN: ... NOT reachable` | `VERIFIED reachable` | +| Where it is recorded | entrypoint boot log | hub log only — nothing agent-side | + +So: **a task whose earlier pushes worked and whose later ones did not is cause 2, not cause 1.** Confirm from the hub log: + +```sh +kubectl -n hive logs deploy/hive | grep -iE 'token.*(refresh|mint)' +``` + +A short task that never pushes successfully at all is cause 1. + +### If neither fits + +Read what the PR-request watcher itself concluded. It probes the repository and then the head ref before blaming a push, and writes its verdict to the request's result file — the shapes and where to find them are in [`hive-open-pr`](hive-open-pr.md#diagnosing-a-pr-request-that-never-opens). + +```sh +kubectl -n hive logs deploy/hive | grep 'pr-request watcher' +``` + ## An agent says "Please run /login" but logging in changes nothing Check whether the same line carries **`API Error: 403`**. If it does, the agent is From 971ac882fe7aba481263ce52fb1aec74d7030d06 Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Tue, 1 Sep 2026 10:05:08 -0400 Subject: [PATCH 017/190] fix(contribute-ws): collapse repeated flaps to one presence row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut retracted the left/released pair but left each flap its own "joined", so 20 flaps still grew the feed by 20 rows — the same eviction #5151 reports, only quieter. Caught by TestReconnectFlap_ManyFlapsDoNotEvictTheFeed, which asserted a row bound rather than the exact shape. The walk now also consumes the superseded "joined" from the previous absorbed flap, so a contributor that never actually left occupies ONE presence row however many times it bounced. Consumption is gated on having already seen a "left" above it, so a "joined" that no departure followed is never touched — pinned by TestReconnectFlap_DoesNotConsumeUnrelatedJoined. Every flap still increments absorbedReconnects, so the count is unaffected by the collapse. Signed-off-by: Andrew Anderson --- .../contribute_reconnect_flap_test.go | 39 ++++++++++++++++--- src/pkg/dashboard/contribute_ws.go | 27 +++++++++++-- 2 files changed, 56 insertions(+), 10 deletions(-) diff --git a/src/pkg/dashboard/contribute_reconnect_flap_test.go b/src/pkg/dashboard/contribute_reconnect_flap_test.go index 1f0d39610..52f089a99 100644 --- a/src/pkg/dashboard/contribute_reconnect_flap_test.go +++ b/src/pkg/dashboard/contribute_reconnect_flap_test.go @@ -64,15 +64,42 @@ func TestReconnectFlap_ManyFlapsDoNotEvictTheFeed(t *testing.T) { flapRows(hub, "alice", "myorg/repo#7") } - got := hub.RecentActivity() - if len(got) > 3 { - t.Fatalf("20 flaps should not fill the feed, got %d rows: %v", len(got), actions(got)) + // 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) } - if got[0].Action != "picked up" { - t.Fatalf("the real 'picked up' row was evicted by flap churn: %v", actions(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", n) + 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) } } diff --git a/src/pkg/dashboard/contribute_ws.go b/src/pkg/dashboard/contribute_ws.go index 0d583f15f..a8352ec07 100644 --- a/src/pkg/dashboard/contribute_ws.go +++ b/src/pkg/dashboard/contribute_ws.go @@ -1066,10 +1066,22 @@ func (h *ContributeWSHub) absorbReconnectFlapLocked(username string) { end := len(h.activity) i := end sawLeft := false - // At most two rows: the "left", then optionally the "released: connection lost" - // that preceded it. Bounded explicitly rather than by a general scan so this can - // never chew through the feed. - for i > 0 && end-i < 2 { + // 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 @@ -1087,6 +1099,13 @@ func (h *ContributeWSHub) absorbReconnectFlapLocked(username string) { 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 From 008a2b8ff3650136f5e10a297f3358465efffb5a Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Tue, 1 Sep 2026 10:06:47 -0400 Subject: [PATCH 018/190] fix(security): stop hive-discord.service executing code planted under /tmp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hive-discord.service runs `node bot.js` with WorkingDirectory=/tmp/hive/discord, so the executed file resolves out of a world-writable parent that is cleared on every reboot. /tmp's sticky bit prevents deleting or renaming entries owned by someone else, but not creating new ones, so a local unprivileged user who wins the post-reboot race — creating /tmp/hive/discord/bot.js before hive-deploy repopulates the checkout — gets code execution as the service user along with the Discord bot token from /etc/hive/discord.env. Requires=hive.service orders startup and validates nothing about who owns the code. Add bin/hive-checkout-guard.sh as an ExecStartPre. It refuses to start unless every directory from / down to the checkout is owned by the service user or root and is not writable by anyone else (a sticky ancestor such as /tmp is accepted; the leaf is not, since the leaf is where a new bot.js would appear), and unless bot.js itself exists, is a regular file rather than a symlink, and is not group- or world-writable. The guard is a script rather than an inline directive on purpose. The obvious one-liner ExecStartPre=/usr/bin/find -maxdepth 0 -user dev ! -perm /go=w -print -quit is a no-op: find exits 0 after a successful traversal whether or not anything matched, so the unsafe case starts the unit anyway. The checkout stays at /tmp/hive. HIVE_REPO_DIR defaults there, but the literal path is also hardcoded in bin/hive.sh, bin/kick-agents.sh and the ExecStart of hive-snapshot.service; relocating it is a deploy-layout change across all of those rather than a fix to this unit, and existing hosts keep their checkout where it is. hive-deploy.sh gains an explicit install block for the guard because both of its sync loops skip files that are not already installed, so a new helper is otherwise never bootstrapped and an upgraded host would pull a unit referencing a script it does not have. PrivateTmp is deliberately not set and ProtectSystem is deliberately not strict: either would hide or freeze the very checkout the unit runs from and stop the bot starting. The contract test pins both as absent so a later hardening pass cannot introduce them. NoNewPrivileges, ProtectSystem=full, ProtectHome=read-only, PrivateDevices and RestrictSUIDSGID are added. src/deploy/test_hive_discord_unit_contract.sh executes the guard against real directory trees and asserts on its exit status, once per attack shape, including the pre-fix arrangement. Wired into v2-ci.yml alongside the other deploy contract tests. Verified failing against the unfixed unit (3 failures) and passing against the fixed one (21 assertions). Fixes #5435 Signed-off-by: Andrew Anderson --- .github/workflows/v2-ci.yml | 13 + CHANGELOG.md | 4 +- bin/hive-checkout-guard.sh | 125 ++++++++++ bin/hive-deploy.sh | 13 + src/deploy/test_hive_discord_unit_contract.sh | 234 ++++++++++++++++++ systemd/hive-discord.service | 33 +++ 6 files changed, 421 insertions(+), 1 deletion(-) create mode 100755 bin/hive-checkout-guard.sh create mode 100755 src/deploy/test_hive_discord_unit_contract.sh diff --git a/.github/workflows/v2-ci.yml b/.github/workflows/v2-ci.yml index b0b2a8933..a57f60218 100644 --- a/.github/workflows/v2-ci.yml +++ b/.github/workflows/v2-ci.yml @@ -494,6 +494,19 @@ jobs: working-directory: . run: bash src/deploy/test_ttyd_url_arg.sh + # #5435: hive-discord.service ran `node bot.js` with + # WorkingDirectory=/tmp/hive/discord, resolving the executed code out of a + # world-writable parent that is cleared on every reboot — a local user who + # created the path first got code execution as the service user holding the + # Discord bot token. The unit now refuses to start via hive-checkout-guard.sh. + # A grep for "ExecStartPre" would pass against a guard that never rejects + # anything (the obvious inline `find ... -print -quit` exits 0 even when it + # matches nothing), so this EXECUTES the guard against real directory trees + # and asserts on its exit status, once per attack shape. + - name: hive-discord.service does not execute planted code (#5435) + working-directory: . + run: bash src/deploy/test_hive_discord_unit_contract.sh + # #4440: the changelog reminder promises it "always succeeds" and "never # blocks a merge", and it did neither on a fork PR — GITHUB_TOKEN is # read-only there, the comment POST 403s, and `bash -e` made that a red diff --git a/CHANGELOG.md b/CHANGELOG.md index 128a22493..b442d19b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,9 @@ Hive did not historically maintain a complete changelog. This file starts a prag ## Unreleased -## 2026-09-01 (v4.0.1) +### Security + +- `hive-discord.service` no longer executes code out of a path an unprivileged local user could have planted ([#5435](https://github.com/kubestellar/hive/issues/5435)). The legacy v1 host unit runs `node bot.js` with `WorkingDirectory=/tmp/hive/discord`, so the executed file resolved out of a world-writable parent that is cleared on every reboot; `/tmp`'s sticky bit prevents replacing entries owned by others but not creating new ones, so a local user who won the post-reboot race — creating `/tmp/hive/discord/bot.js` before `hive-deploy` repopulated the checkout — got code execution as the service user together with the Discord bot token from `/etc/hive/discord.env`. The unit now runs `bin/hive-checkout-guard.sh` as an `ExecStartPre` and refuses to start unless every directory from `/` down to the checkout is owned by the service user or root and not writable by anyone else (a sticky ancestor such as `/tmp` is accepted, the leaf is not), and unless `bot.js` itself exists, is a regular file rather than a symlink, and is not group- or world-writable. `NoNewPrivileges`, `ProtectSystem=full`, `ProtectHome=read-only`, `PrivateDevices` and `RestrictSUIDSGID` are set alongside it. **The checkout stays at `/tmp/hive` and no path changes**, so existing hosts keep working: `/tmp/hive` is hardcoded well beyond `HIVE_REPO_DIR` (in `bin/hive.sh`, `bin/kick-agents.sh`, and the `ExecStart` of `hive-snapshot.service`), and relocating it is a deploy-layout change rather than a fix to this unit. `PrivateTmp` is deliberately *not* set and `ProtectSystem` is deliberately not `strict` — either would hide or freeze the very checkout the unit runs from and stop the bot starting; the contract test pins both as absent so a later hardening pass cannot introduce them. Containerized deployments never used this unit and are unaffected. ### Added diff --git a/bin/hive-checkout-guard.sh b/bin/hive-checkout-guard.sh new file mode 100755 index 000000000..930b3baa8 --- /dev/null +++ b/bin/hive-checkout-guard.sh @@ -0,0 +1,125 @@ +#!/bin/bash +# hive-checkout-guard.sh — refuse to let a systemd unit execute code out of a +# checkout that an unprivileged local user could have planted (#5435). +# +# Usage: hive-checkout-guard.sh [entrypoint-file ...] +# +# WHY THIS EXISTS. Units like hive-discord.service run with +# WorkingDirectory=/tmp/hive/... and ExecStart=/usr/bin/node bot.js, so the code +# they execute is resolved out of a world-writable parent. /tmp's sticky bit +# only prevents deleting or renaming entries owned by SOMEONE ELSE; it does not +# prevent CREATING new ones. /tmp is also cleared on reboot, so after every boot +# there is a window in which /tmp/hive does not exist and any local user can +# create it — along with the file the unit is about to execute — before +# hive-deploy repopulates the real checkout. The service would then run attacker +# code as the service user, holding whatever EnvironmentFile grants it. +# +# Ordering directives do not help: Requires=/After= sequence startup, they say +# nothing about who owns the files. +# +# WHY A SCRIPT RATHER THAN INLINE ExecStartPre. The obvious one-liner +# +# ExecStartPre=/usr/bin/find -maxdepth 0 -user dev ! -perm /go=w -print -quit +# +# is a NO-OP GUARD: find exits 0 after a successful traversal whether or not +# anything matched, so the "unsafe" case exits 0 too and systemd starts the unit +# anyway. This script's exit status is the assertion, and every failure path +# below exits non-zero, so a violation actually stops ExecStart from running. +# +# Test: bash src/deploy/test_hive_discord_unit_contract.sh +set -uo pipefail + +DIR="${1:-}" +shift || true + +die() { echo "hive-checkout-guard: REFUSING to start: $*" >&2; exit 1; } + +# Portable stat helpers. GNU coreutils and BSD/macOS stat take different flags, +# and they disagree on the permission format: GNU '%a' includes the setuid/ +# setgid/sticky bits when set (e.g. 1777), BSD '%Lp' prints only the low three +# (777), dropping sticky entirely. Inferring sticky from the digit count is +# therefore WRONG on BSD — it silently reports every sticky directory as +# non-sticky, which would reject /tmp itself. Sticky is read separately below +# via `test -k`, which both platforms implement. +stat_owner() { stat -c '%u' "$1" 2>/dev/null || stat -f '%u' "$1" 2>/dev/null; } +stat_perms() { stat -c '%a' "$1" 2>/dev/null || stat -f '%Lp' "$1" 2>/dev/null; } + +[ -n "$DIR" ] || die "no directory argument given" + +# The service user this guard is asserting for. Resolved from the running EUID +# rather than hardcoded, so the check follows the unit's User= automatically. +ME="$(id -u)" + +# Resolve without following a final symlink into somewhere else: a symlinked +# /tmp/hive/discord pointing at an attacker-owned tree would otherwise pass the +# ownership test on its target while the unit executes the target's code. +[ -L "$DIR" ] && die "$DIR is a symlink — refusing to execute out of it" +[ -d "$DIR" ] || die "$DIR does not exist or is not a directory" + +# Every component from / down to DIR must be safe. Checking only the leaf is not +# enough: if /tmp/hive is attacker-owned they can swap the whole discord/ +# subtree, and a leaf-only check would happily validate the replacement. +node="$(cd "$DIR" 2>/dev/null && pwd -P)" || die "cannot resolve $DIR" +while : ; do + owner="$(stat_owner "$node")" || die "cannot stat $node" + perms="$(stat_perms "$node")" || die "cannot stat $node" + [ -n "$owner" ] && [ -n "$perms" ] || die "cannot stat $node" + + # Owned by us or by root. root is accepted because the deploy tooling runs + # some steps under sudo; anything else means a third party controls the path. + if [ "$owner" != "$ME" ] && [ "$owner" != "0" ]; then + die "$node is owned by uid $owner (expected $ME or root)" + fi + + # Group- or other-writable is only tolerable with the sticky bit, which is + # what makes /tmp itself acceptable as an ANCESTOR: sticky means other users + # cannot replace entries they do not own. A non-sticky world-writable + # directory anywhere on the path means the tree can be swapped underneath us. + # + # perms is 3 or 4 octal digits depending on platform; take the low three as + # the mode and read sticky separately (see stat_perms above). + mode="${perms: -3}" + sticky=0 + [ -k "$node" ] && sticky=1 + go_w=0 + case "${mode:1:1}" in 2|3|6|7) go_w=1 ;; esac + case "${mode:2:1}" in 2|3|6|7) go_w=1 ;; esac + + if [ "$go_w" = 1 ] && [ "$sticky" != 1 ]; then + die "$node is group/other-writable ($perms) without the sticky bit" + fi + + # The LEAF must not be world-writable at all, sticky or not. Sticky protects + # existing entries from replacement, but the leaf is where new files appear, + # and a new bot.js is exactly the attack. + if [ "$node" = "$(cd "$DIR" && pwd -P)" ] && [ "$go_w" = 1 ]; then + die "$DIR is group/other-writable ($perms) — anyone could add files to it" + fi + + [ "$node" = "/" ] && break + parent="$(dirname "$node")" + [ "$parent" = "$node" ] && break + node="$parent" +done + +# Each named entrypoint must exist, be a regular file, be owned by us or root, +# and not be writable by anyone else. +for f in "$@"; do + path="$DIR/$f" + [ -L "$path" ] && die "$path is a symlink — refusing to execute it" + [ -f "$path" ] || die "$path does not exist or is not a regular file" + + owner="$(stat_owner "$path")" || die "cannot stat $path" + perms="$(stat_perms "$path")" || die "cannot stat $path" + [ -n "$owner" ] && [ -n "$perms" ] || die "cannot stat $path" + + if [ "$owner" != "$ME" ] && [ "$owner" != "0" ]; then + die "$path is owned by uid $owner (expected $ME or root)" + fi + + mode="${perms: -3}" + case "${mode:1:1}" in 2|3|6|7) die "$path is group-writable ($perms)" ;; esac + case "${mode:2:1}" in 2|3|6|7) die "$path is world-writable ($perms)" ;; esac +done + +exit 0 diff --git a/bin/hive-deploy.sh b/bin/hive-deploy.sh index e13bb14b8..f63707fd0 100755 --- a/bin/hive-deploy.sh +++ b/bin/hive-deploy.sh @@ -103,6 +103,19 @@ if [ -f "$BASELINE_HELPER_SRC" ] && ! cmp -s "$BASELINE_HELPER_SRC" "$BASELINE_H SYNCED="$SYNCED hive-baseline-check.sh" fi +# Same bootstrap problem as the baseline helper above: hive-discord.service's +# ExecStartPre references /usr/local/bin/hive-checkout-guard.sh (#5435), and +# neither sync loop above can create it — both skip any file that is not +# already installed. Without this block an upgraded host would pull a unit that +# calls a script it does not have, and systemd would refuse to start the bot. +# Install it BEFORE the unit files are reinstalled by kick-agents.sh. +CHECKOUT_GUARD_SRC="$HIVE_REPO/bin/hive-checkout-guard.sh" +CHECKOUT_GUARD_DST="$INSTALL_DIR/hive-checkout-guard.sh" +if [ -f "$CHECKOUT_GUARD_SRC" ] && ! cmp -s "$CHECKOUT_GUARD_SRC" "$CHECKOUT_GUARD_DST" 2>/dev/null; then + sudo install -m 0755 "$CHECKOUT_GUARD_SRC" "$CHECKOUT_GUARD_DST" + SYNCED="$SYNCED hive-checkout-guard.sh" +fi + # hive.sh is installed as /usr/local/bin/hive (no .sh extension) HIVE_CLI="$HIVE_REPO/bin/hive.sh" HIVE_INSTALLED="$INSTALL_DIR/hive" diff --git a/src/deploy/test_hive_discord_unit_contract.sh b/src/deploy/test_hive_discord_unit_contract.sh new file mode 100755 index 000000000..bb5de0ae5 --- /dev/null +++ b/src/deploy/test_hive_discord_unit_contract.sh @@ -0,0 +1,234 @@ +#!/usr/bin/env bash +# hive-discord.service must not execute code out of a directory a local user +# could have planted (#5435). +# Run: bash src/deploy/test_hive_discord_unit_contract.sh +# +# WHAT THE BUG WAS. The unit ran: +# +# WorkingDirectory=/tmp/hive/discord +# ExecStart=/usr/bin/node bot.js +# EnvironmentFile=/etc/hive/discord.env +# +# so bot.js resolved out of a world-writable parent that is CLEARED ON REBOOT. +# /tmp's sticky bit only stops a user from deleting or renaming entries owned by +# someone else; it does not stop them creating /tmp/hive/discord/bot.js in the +# window between the boot that wipes /tmp and hive-deploy repopulating the +# checkout. The winner of that race gets code execution as `dev` holding the +# Discord bot token. Requires=hive.service orders startup and validates nothing. +# +# WHY THIS TEST RUNS THE GUARD INSTEAD OF GREPPING THE UNIT. A grep for +# "ExecStartPre" would pass against a guard that never rejects anything — and +# that is not hypothetical here. The obvious inline form, +# +# ExecStartPre=/usr/bin/find -maxdepth 0 -user dev ! -perm /go=w -print -quit +# +# exits 0 whether or not it matched, so the unsafe case starts the unit anyway: +# a no-op guard that greps green, which is exactly the failure #5398 found in +# another contract test. So the assertions below EXECUTE bin/hive-checkout-guard.sh +# against real directory trees and assert on its EXIT STATUS, once per attack +# shape, including the pre-fix arrangement which must be rejected. +set -uo pipefail + +PASS=0 +FAIL=0 +pass() { echo " PASS: $1"; PASS=$((PASS + 1)); } +fail() { echo " FAIL: $1"; [ $# -gt 1 ] && echo " $2"; FAIL=$((FAIL + 1)); } + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +UNIT="${ROOT}/systemd/hive-discord.service" +GUARD="${ROOT}/bin/hive-checkout-guard.sh" + +echo "=== hive-discord.service does not execute planted code (#5435) ===" + +for f in "$UNIT" "$GUARD"; do + if [ ! -f "$f" ]; then + fail "locate $f" "the layout moved — this test cannot verify anything" + echo "" + echo "=== Results: $PASS passed, $FAIL failed ===" + exit 1 + fi +done + +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT + +# --- the unit wires the guard in --------------------------------------------- +echo "" +echo "--- the unit file ---" + +# Read the directive out of the unit rather than restating it, so a unit that +# drops the guard fails here instead of silently passing the behaviour tests. +GUARD_LINE="$(grep -E '^ExecStartPre=' "$UNIT" || true)" +if [ -z "$GUARD_LINE" ]; then + fail "hive-discord.service has an ExecStartPre guard" \ + "without one, systemd executes whatever bot.js is present at start time" +else + pass "hive-discord.service has an ExecStartPre guard" + if printf '%s' "$GUARD_LINE" | grep -q 'hive-checkout-guard.sh'; then + pass "the guard is hive-checkout-guard.sh (exit status is the assertion)" + else + fail "the guard is hive-checkout-guard.sh" "got: ${GUARD_LINE}" + fi + # The guard must be told which file ExecStart will run. Checking the directory + # alone would let a missing/planted bot.js through. + EXEC_FILE="$(grep -E '^ExecStart=' "$UNIT" | sed 's/.* //')" + if printf '%s' "$GUARD_LINE" | grep -qF -- "$EXEC_FILE"; then + pass "the guard checks the file ExecStart actually runs (${EXEC_FILE})" + else + fail "the guard checks the file ExecStart runs (${EXEC_FILE})" "got: ${GUARD_LINE}" + fi +fi + +# PrivateTmp would give the unit a private /tmp, hiding the very checkout +# WorkingDirectory points at — the service would fail to start. Asserting its +# ABSENCE keeps a future "more hardening is better" edit from breaking the bot. +if grep -qE '^PrivateTmp=(yes|true|1)' "$UNIT"; then + fail "PrivateTmp is not set on this unit" \ + "WorkingDirectory is under /tmp; a private /tmp hides the checkout and the unit cannot start" +else + pass "PrivateTmp is not set (it would hide the /tmp checkout this unit runs from)" +fi + +for d in NoNewPrivileges ProtectSystem; do + if grep -qE "^${d}=" "$UNIT"; then + pass "${d} is set" + else + fail "${d} is set" "unit lost a hardening directive" + fi +done + +# ProtectSystem=strict would make /tmp read-only for the service. Same trap. +if grep -qE '^ProtectSystem=strict' "$UNIT"; then + fail "ProtectSystem is not 'strict'" "strict mounts /tmp read-only; this unit runs out of /tmp" +else + pass "ProtectSystem is not 'strict' (which would make the /tmp checkout read-only)" +fi + +# --- the guard actually rejects things --------------------------------------- +echo "" +echo "--- the guard, run against real trees ---" + +# Build a tree shaped like the real one: a sticky world-writable ancestor +# standing in for /tmp, with the checkout underneath it. +# $WORK/tmp/hive/discord/bot.js +mk_tree() { + rm -rf "${WORK}/tmp" + mkdir -p "${WORK}/tmp/hive/discord" + chmod 1777 "${WORK}/tmp" + chmod 755 "${WORK}/tmp/hive" "${WORK}/tmp/hive/discord" + : > "${WORK}/tmp/hive/discord/bot.js" + chmod 644 "${WORK}/tmp/hive/discord/bot.js" +} + +# expect