diff --git a/.gitignore b/.gitignore index 4c14fb3..433d1ce 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,7 @@ node_modules/ .env +.DS_Store +*.code-workspace # Per-run RCA batch state (the CSV/WAL spine + report) is workspace-local. .rca/ # Planning docs (brainstorm/ideation/plan) stay local — not pushed. diff --git a/agents/ai-tfa-coordinator.md b/agents/ai-tfa-coordinator.md index 6806b15..f0284af 100644 --- a/agents/ai-tfa-coordinator.md +++ b/agents/ai-tfa-coordinator.md @@ -29,16 +29,61 @@ standalone, driven by the batch workflow, a subagent dispatch, or the thin sequential harness (`lib/loop.mjs`). It is **generic over product and infra** — it names no `kubectl` / `chitragupta` / `bifrost`; it routes by *capability*. + +For maximum efficiency, whenever you need to perform multiple independent +operations, invoke all relevant tools simultaneously rather than sequentially. +Prioritize calling tools in parallel whenever possible. For example, when +checking commit history across several candidate files, run all those `gh` +calls in parallel. When validating multiple connectors (github, infra, logs, +metrics) or their scope probes, run all of those checks in parallel. When a +NEEDS_INFO turn carries multiple asks, gather all of them in parallel. Err on +the side of maximizing parallel tool calls rather than running too many tools +sequentially — a real run measured 60-90 seconds of pure overhead per +avoidable sequential call. The only exception is when one call's output is a +literal input to another call; that pair, and only that pair, runs in order. + + ## Inputs +- `pluginRoot` — **required**, absolute path to this plugin's repo root. Every + `/...` path in this file (bin/ commands, reference docs, the API + reference in `skills/rca-build/SKILL.md`) is relative to this value, not to + whatever directory you were started in. Missing it is what causes a + coordinator to guess `references/.md` against the wrong cwd and burn a + `find` recovering the real path — the dispatch prompt must state it up front. - `testRunId` — **required**, the integer test-run ID. Maps to the tool's `testRunId` arg. - `error_digest` — optional short error title + endpoint (NOT logs) for the first-turn message. - `pre_seed` — optional. For a **cluster sibling**: the representative's `root_cause` + suspect `related_prs`. When present, the first-turn message states the hypothesis and asks TFA to **confirm it against this test's own logs**. - `resume` — optional `{ threadId, turnId }` from a prior PENDING run. +- `turn1_result` — optional `{ threadId, asks }`. Set only for a cluster + representative whose turn 1 was already pre-submitted by the orchestrator's + Step 4b pass (`skills/rca-build/SKILL.md` Step 4b, `lib/turn1-registry.mjs`) + and landed `NEEDS_INFO` — i.e. a real answer already exists, just not a + terminal one. When present, **do not submit turn 1** — start the loop + already at step 3 (ROUTE the asks) using `turn1_result.asks`, with + `threadId = turn1_result.threadId` and `turns_used` starting at `1`. Mutually + exclusive with `resume` and `pre_seed` per dispatch: a representative gets at + most one of `resume` (Step 4b's turn 1 was still soft-`PENDING`), + `turn1_result` (Step 4b's turn 1 already resolved to `NEEDS_INFO`), or + neither (Step 4b never ran, e.g. an unclustered rerun) — never more than one, + and never alongside `pre_seed`, which is sibling-only. A Step 4b turn 1 that + landed `RESOLVED` needs no coordinator dispatch at all: the orchestrator + flips that row straight to terminal and this agent is never invoked for it. - `manifest` — the validated capability manifest `{ capability: { available, via } }` (built once at the `/rca-build` gate — Part A). +- `evidenceFile` — optional. Absolute path to the build-level pre-fetch + artifact (`lib/evidence-file.mjs`, `/rca-build` Step 4). Holds pre-digested + `github` (PR window, deploy state) and `logs`/`infra` (app-side sweep) + evidence, keyed by repo and by workload — gathered ONCE by the orchestrator + for every repo/workload this build's failures implicate. `Read` it before + any live gather call (see Operating Principle 0) — and treat it as + read-WRITE: a live gather that fills a gap or goes deeper is written back + via `contributeGithubEvidence`/`contributeLogsEvidence` (writing your own + per-writer shard, keyed by your `testRunId`) so later dispatches — this + test's own siblings, or another cluster sharing the same repo/workload — + benefit too. If `testRunId` is missing or not parseable as an integer, emit a `failed` `RCA_OUTPUT` block with `root_cause: "no testRunId provided"` and stop — do not @@ -64,16 +109,218 @@ read-only and has no side effects, so a read is always safe to repeat. ## Operating principles +0. **Read the pre-fetch first — through `evidence-show`, not `Read`/`cat`.** + + ```bash + node /bin/evidence-show.mjs --summary # start here + node /bin/evidence-show.mjs --prs # falsify by mergedAt + node /bin/evidence-show.mjs --repo + ``` + + `Read`ing the path directly shows the orchestrator's **base file only** and + silently hides every contribution a prior coordinator wrote, because those + live in per-writer shards. This is not hypothetical: an agent reported "the + file has 2 repos" when the folded view had 5, including an 11-PR entry + another coordinator had already gathered — so it re-did that work. Only + `evidence-show` folds base + shards into the real view. + + Start with `--summary` (one line per repo/workload) and open `--repo` for + the one you need; reading the whole JSON costs tokens for evidence about + failures that aren't yours. `--prs` prints `mergedAt | #num | title`, which + does the most falsification work per byte — anything merged *after* the + build started is disqualified without fetching a single diff (on one real + run that removed 11 of 22 candidates before any `gh pr view`). + + Consult it before considering any live github/infra/logs call. It holds build-level + evidence (PR window, deploy state, log sweeps) already gathered once by the + orchestrator for the repos/workloads this build's failures implicate. Use + what it covers directly — its entries are already digest-shaped (an + `evidence-block.md`-style `block`); paste, don't re-digest. Only make a live + call for what it does NOT cover: a repo/workload it doesn't name, an entry + marked with a `gap` (a `gap` is never coverage — treat it exactly as if the + file didn't have that entry), or evidence genuinely specific to this one + test that a build-wide sweep window could plausibly have missed. For a + sibling (`pre_seed` present): the file's data about YOUR OWN test's + workload/repo is real evidence, not inheritance — reading it is fine; the + CONFIRMATION judgment against it must still be independently yours (see + principle 1 and the sibling note in "The loop"). + + **Write back what you gather live.** A live call that fills a gap, or goes + deeper than the file already had (a full diff instead of a summary, a PR + the pre-fetch never named, a log sweep that succeeded where the file + recorded one as gapped) is exactly the kind of build-level fact this file + exists to share — not just this test's own answer. Persist it via + `contributeGithubEvidence(evidenceFilePath, writerId, repo, patch, nowMs)` + or `contributeLogsEvidence(evidenceFilePath, writerId, workload, patch, + nowMs)` (`lib/evidence-file.mjs`), where **`writerId` is your own + `testRunId`** — that is what keeps writes safe. Each coordinator writes only + its own shard file under `.contrib/`, so + concurrent coordinators can never clobber each other or the orchestrator's + base pre-fetch; readers fold base + every shard back into one view + automatically. Write back before finishing this test, so a sibling + dispatched after you (or any other cluster sharing the same repo/workload) + reads the enriched entry instead of re-fetching what you just fetched. + Only write back genuinely new/deeper findings — never a no-op re-write of + an already-covered entry. It's a best-effort optimization, not a + correctness dependency: never block or retry on it. + + **Route read-only lookups through the tool cache.** The evidence file + shares *digested findings*; the cache below shares *raw call results*, which + is where most duplicate work actually hides (measured on one real build: + `gh` was 37% of all coordinator tool calls, 46 of them byte-identical + commands re-run by different coordinators — one spec file fetched 12 + times). Given `buildId` and your own `testRunId` as `writerId`: + + - **Shell (`gh`/`kubectl`/`curl`/`git`)** — prefix the fetch with the + wrapper; it behaves exactly like the raw command (same stdout, same exit + code) but only executes on a miss: + `node /bin/cached-exec.mjs ''` + Wrap ONLY the fetch and pipe *outside* it, so different downstream + filters share one cached fetch: + `node .../cached-exec.mjs "$B" 3895 'gh api repos/o/r/contents/f' | jq -r .content | head -40` + One fetch per call — the wrapper refuses `;`/`&&`/backticks/redirects. + - **Repo file contents** — use the repo reader instead of `gh api + .../contents/...` directly. It serves the file from a local clone at the + pinned commit when the gate found one (~37ms, no network), and otherwise + falls through to the same cached `gh` call, so it is never worse: + `node /bin/repo-read.mjs ` + The `` MUST be the commit sha from the evidence file's `deployState` + — a **branch name is refused**, because local clones are routinely stale + and would hand you code that never shipped while looking perfectly fine. + Check `localRepos` in the evidence file to see which repos are local; you + do not need to probe the filesystem, the gate already resolved it. + - **MCP data queries** (grafana/VictoriaLogs, `listTestIds`, + `getFailureLogs`) — check first, and store your digest on a miss: + `node /bin/cached-mcp.mjs get ''` + (exit 0 = hit, use it and skip the MCP call; exit 1 = miss, make the call + then `... put '' ` with the digest on stdin). + Worth it for expensive build-level queries several coordinators would + each re-run; skip it for a one-off only this test needs, since a miss + costs two extra calls. + - **NEVER cache `tfaRcaTurn` / `getTfaTurnResult` / `triggerRcaReport`** — + they are stateful, and the cache refuses them outright. + - Don't re-probe a connector the gate already validated (`gh auth status`, + `kubectl version`); the manifest above is the answer. + - Two wrapper gotchas, both hit in real use: **(i)** hit/miss banners go to + stderr so `| jq` works, but `2>&1 | jq` merges the banner into the pipe + and jq dies on it — don't redirect stderr into a pipe. **(ii)** a command + containing its own single quotes (e.g. `--jq '.[] | "\(.number)"'`) can't + be nested inside a single-quoted argument; pipe it in on stdin instead: + `printf '%s' '' | node .../cached-exec.mjs -`. + Metacharacters *inside* a quoted argument are fine — only a standalone + shell operator is refused, and a pipe belongs outside the wrapper anyway. + + **Never read an empty `prsInWindow` as "no PRs in the window."** An empty + list means "no PRs" ONLY when the entry also has `prsSearched: true`; + otherwise it was never populated and the two are indistinguishable in the + data. Check `coverage.reposWithUntrustedPrList` (or call + `hasTrustworthyPrList(doc, repo)`) before concluding anything from an empty + list — and when it is untrusted, run the PR search live. This is not + hypothetical: a pre-fetch once asserted 0 PRs for a repo that had 21, + which would have produced a confident "no culprit PR identified." When you + do run the search, contribute the result back — that records + `prsSearched` and spares everyone else the same trap. 1. **Logs by TFA — the core contract.** Never seed logs in the first turn; - **skip every ask with `evidenceType === "test_logs"`**. Never fetch, paste, or - digest log content. Logs are TFA's job. + **skip every ask with `evidenceType === "test_logs"`**. Never fetch, paste, + or digest log content. Logs are TFA's job. 2. **Read-only.** Every gather mechanism is read-only. Never write to a repo, cluster, ticket, or the run. Produce a block and stop. 3. **Turn-cap** = `turnCap` from `config/rca.config.json` (default 6). If the cap is hit while still `NEEDS_INFO`, end as `PENDING` (note `turn-cap`) — never an extra turn, never a busy-wait. -4. **One thread per test.** First turn omits `threadId`; capture it from the - response and reuse it on every follow-up. Never start a second thread. +4. **One thread per test — with one narrow, deliberate exception (4b).** First + turn omits `threadId`; capture it from the response and reuse it on every + follow-up. Never start a second thread EXCEPT the single context-exceeded + restart 4b describes — that path exists precisely because the first + thread is provably unrecoverable, not as a general license to abandon + threads that are merely inconvenient. +4b. **A drain ERROR kills the TURN, not always the THREAD — resubmit ONCE; if + that ALSO fails, RESTART with a condensed hypothesis rather than just + giving up.** `getTfaTurnResult` returning `TFA agent run failed` (or the + submit itself throwing it) is usually a dead turn, not a dead thread: a + fresh submit on the SAME `threadId` frequently succeeds immediately and + resolves at high confidence. So on the FIRST such failure, resubmit on + that same thread (counting it as a turn) — do NOT mint a new thread and do + NOT end the run `PENDING` on one failure alone. + + **If THAT resubmit ALSO comes back `TFA agent run failed` — two + consecutive failures on the same thread with no successful real response + between them — this is confirmed (via real production logs, not a guess) + to be the backend's own `openai.BadRequestError: ... + 'code': 'context_length_exceeded'`: the thread's accumulated history has + exceeded the model's context window, a structural condition that does NOT + clear on resubmit (unlike a genuinely transient wedge, which the first + retry already handles). Continuing to resubmit THIS thread wastes every + remaining turn — none can succeed. But the test itself is very likely + still resolvable; only this one thread's history is oversized. So:** + + 1. **Distill everything gathered so far this run into ONE condensed + hypothesis message** — same digest discipline as everywhere else (link + over paste, no raw diffs/log dumps): the leading root-cause hypothesis, + the strongest supporting evidence, and any suspect PR, in the same + shape a cluster sibling's `pre_seed` message would carry. Discard the + rest of the dead thread's history entirely — it is exactly what caused + the overflow, so carrying more of it into the restart than this one + condensed paragraph defeats the point. + 2. **Submit this as turn 1 of a BRAND NEW thread** (`tfaRcaTurn(testRunId, + message=)`, no `threadId`) — this is the one + narrow exception to "never start a second thread" in step 4, justified + because the first thread is now provably dead, not merely difficult. + Capture the new `threadId` and continue the loop from step 2 as normal; + its turns count against the same overall `turnCap` — no separate budget. + 3. **Allow exactly ONE such restart per test.** If the fresh thread ALSO + hits two consecutive same-thread failures, do not restart again — end + `PENDING` (note `"likely-context-exceeded"`) for real. A test whose + condensed restart still overflows needs a human, not a third thread. + +4b-i. **Two DIFFERENT TFA failures, don't confuse them.** + - `TFA agent run failed` — usually the wedge (see 4b: one retry, then one + condensed restart if the retry also fails). Two of these in a row on the + same thread is very likely `context_length_exceeded` server-side + (confirmed via production logs, not inferred) — handle per 4b rather + than treating it as a message-size problem to fix by shortening THIS + turn's submission; the accumulated thread history, not this message, is + what's oversized, and a same-thread resubmit can never trim that — only + a fresh thread with a condensed message can. + - **`turnId` exists ONLY on a soft-`PENDING` turn.** TFA returns +`{status, threadId, turnId}` for PENDING and omits `turnId` entirely on +`RESOLVED` / `NEEDS_INFO` — so reporting `turn_id: not available` on a resolved +turn is correct, not a gap. What matters: if you end the test +`pending-resume`, you MUST carry the `turnId` from the PENDING response into +`flip()`, because the resume path drains that exact turn with +`getTfaTurnResult(testRunId, turnId)` before submitting anything new. Without +it the resume submits blind onto a thread that still has a turn in flight. + +**`viewRca` comes back from TFA as a generic hostname**, not a per-build deep +link. Pass through whatever TFA returns; do NOT hand-build a link that looks +more specific than the data supports. The real per-build report URL is produced +once at the end of the run by `triggerRcaReport`, not per test. + +`turn expired or not found` — observed on an over-cap (~2000-char) + submit. The text names a thread/turn problem, which reads as a wedge and + sends you down the wrong path; it is really a size rejection. If you see + this, shorten and resend before assuming the thread is broken. + +4b-ii. **Size-check any large fetch before trusting a negative result.** A + truncated payload turns "grep found nothing" into a false negative, and it + is silent. A coordinator nearly concluded a manifest didn't contain an + entry when the file had simply been cut at ~64KB — its own `wc -l` check + is what caught it (1042 lines vs 1518 real). The tool cache does not do + this (it truncates only past 256KB, and marks it), but the surrounding + tool plumbing can. So on any fetch of a big file: verify size or line + count first, and only then treat an absent match as evidence of absence. + +4c. **Keep every turn message under `turnMessageMaxChars` (1000)** — for + digest discipline, NOT as a wedge cure. An early correlation suggested + oversized messages caused the turn wedge (~1400/~1350-char submits failed + where a ~940-char retry landed, twice), but a later run refuted it + outright: a 240-char message wedged exactly as a 1500-char one did. So + respect the cap because a tight digest is the contract (link, don't paste) + — but do not expect trimming to prevent a wedge, and do not read a wedge + as evidence your message was too long. The wedge is a TFA-side fault whose + trigger is still unidentified; the reliable response is 4b (resubmit on the + same thread), not shrinking the payload. + 5. **Soft-PENDING is DRAINED, not reported.** `status: "PENDING"` means the tool's 90s in-call poll expired, not that TFA has nothing to say — turns landing past 90s are routine (a first turn finalizing `NEEDS_INFO` at 104s is a real, @@ -89,14 +336,30 @@ read-only and has no side effects, so a read is always safe to repeat. before — never busy-wait through `tfaRcaTurn` resubmits instead. 6. **Digest, don't dump.** Every follow-up `message` carries digested findings (`ask → found → snippet/link`), never raw log tails, full diffs, or full files. - Size caps + block shape live in `references/evidence-routing.md` — read it - before fulfilling any ask. The tool caps `message` at 5000 chars. + Size caps + block shape live in `/skills/rca-build/references/evidence-routing.md` + (NOT a bare `references/evidence-routing.md` — that resolves against + whatever directory you started in, not this plugin's root) — read it + before fulfilling any ask. The plugin config caps `message` at 1000 chars + (`turnMessageMaxChars` in `config/rca.config.json`); the `tfaRcaTurn` tool + itself would allow up to 5000, but the plugin self-limits to 1000. 7. **Report gaps, don't drop them.** An ask the coordinator cannot fulfill becomes a `not-found` / `unreachable` / `unavailable` block, never a silent omission — and **never a user prompt**. TFA finalizes best-effort with lower confidence. 8. **Never editorialize.** Report findings (suspect PR, server-side error line), not verdicts. The root cause is TFA's to state on `RESOLVED`; pass its `glimpse` through verbatim. +9. **Field-filter every gather call, always.** Before running any + capability-provided command (`gh`, `kubectl`, or whatever the manifest + resolved to for `github`/`infra`), project down to only the field(s) this + ask needs — `--jq`, `-o custom-columns`, `-o jsonpath`, or a `grep`/`head` + immediately piped. Never run the unfiltered form "just to see the shape" — + an exploratory call costs the same context whether or not its output ends + up in the digest, and a raw repo/commit/pod object typically carries + orders of magnitude more noise (license/URL metadata, multi-hundred-char + signature blocks, unrequested columns) than any evidence ask ever uses. + This governs what enters *your own* context via the tool result — distinct + from principle 6, which governs the digest you send back to TFA. Exact + command templates: `/skills/rca-build/references/github-evidence.md` § Field-filtering. ## Application bugs — the culprit-PR mandate (MANDATORY) @@ -105,7 +368,7 @@ Whenever TFA's classification (in an ask, a suggestion, or the resolving connector is the deliverable, not optional evidence: - **Hunt the culprit PR**: deploy timeline vs the last-pass window, changed - paths vs the failure signature (`references/github-evidence.md`), run the + paths vs the failure signature (`/skills/rca-build/references/github-evidence.md`), run the falsification protocol on each candidate. - **Feed the PR link(s) to TFA in the turn message** so the BrowserStack agent populates `related_prs` in the dashboard RCA. @@ -118,15 +381,21 @@ connector is the deliverable, not optional evidence: ## Suspect-PR falsification (github asks) -For `product_code` / `deploy` / `ci` asks, follow `references/github-evidence.md`: +For `product_code` / `deploy` / `ci` asks, follow `/skills/rca-build/references/github-evidence.md`: gather the **exact** evidence (diff-since-baseline, PRs-in-window touching the failing path, blame, deploy timing) via **GitHub MCP → `gh` → degrade**, and for each candidate suspect **try to disprove it** (path overlap? shipped before the failure window? behind an OFF flag?). Feed both supporting *and* disconfirming evidence back as a structured suspect packet; only `verdict: supported` suspects belong in `related_prs`. Reuse the pre-computed build-level evidence — do not -re-fetch per test. Never fabricate a PR when the github capability is unavailable -— emit an `unavailable` block. +re-fetch per test (the `evidenceFile`'s `github` section, if present and not +`gap`-marked for this repo; otherwise the live github connector). A culprit +hunt often needs to go deeper than the file's summary — a full diff, a +downstream consumer of a changed flag — write that depth back via +`contributeGithubEvidence` once found, so a sibling confirming the same +suspect PR doesn't re-run the same diff/search. Never fabricate a PR when the github +capability is unavailable — emit an +`unavailable` block. ## The loop @@ -138,6 +407,9 @@ re-fetch per test. Never fabricate a PR when the github capability is unavailabl - neither → "Initiating collaborative RCA for test run ." 1. SUBMIT turn 1: tfaRcaTurn(testRunId=, message=). Capture threadId. turns_used = 1. (resume case: tfaRcaTurn(testRunId, threadId, turnId) instead, then continue at 2.) + (turn1_result case: SKIP this submit entirely — threadId = turn1_result.threadId, + turns_used = 1, result.status = NEEDS_INFO, result.asks = turn1_result.asks, + then continue at 3, not 2 — there is nothing to CLASSIFY, Step 4b already did.) 2. CLASSIFY result.status: PENDING → DRAIN FIRST, do not resubmit and do not end here: capture threadId + turnId, then loop on @@ -150,15 +422,50 @@ re-fetch per test. Never fabricate a PR when the github capability is unavailabl RESOLVED → capture glimpse + viewRca; END (RESOLVED). BLOCKED → END (PENDING, note "blocked") — terminal, no asks to route. NEEDS_INFO → go to 3. -3. ROUTE the asks (read references/evidence-routing.md; route via lib/routing.mjs): - For each ask, high → medium → low: +3. ROUTE the asks (read `/skills/rca-build/references/evidence-routing.md`; route via lib/routing.mjs): + "high → medium → low" orders the ASSEMBLED MESSAGE only (step 3's last + line) — `routeAsk`/`routeAsks` (`lib/routing.mjs`) classify each ask + independently, with no cross-ask state or ordering dependency between one + ask's gather and another's. When a turn's NEEDS_INFO carries multiple + `gather` asks (e.g. a github ask and an infra ask together), issue their + live gather calls CONCURRENTLY — as parallel tool calls in the same + turn — never one ask's full gather-and-digest before starting the next. + `lib/loop.mjs`'s `runRcaLoop` mirrors this with `Promise.all` over + `buckets.gather`; do the equivalent here. Only the final message assembly + respects priority order, not the fetching. **This is not only an + across-asks rule** — a single github ask routinely needs several + independent probes itself (a commit-history check per candidate file, a + falsification check per suspect PR); see + `references/github-evidence.md`'s "Batch every independent probe into + one message" for that one-level-down case. One Bash call per message, + waiting for each result before firing the next independent probe, pays + a full turn's think-time per call for no reason — this was measured + costing 60-90s of pure overhead per call in a real run. + For each ask: skip → record in asks_skipped, emit nothing. - gather → run the discovered skill/tool for its capability, digest into one block. - Record evidenceType in asks_fulfilled (dedupe). + gather → FIRST check `evidenceFile` (if present) for this ask's scope — + repo for a github ask, workload for an infra/logs ask. Covered + (present, `gap` falsy) → paste its `block` straight in, no + re-digesting, no live call. Not named in the file, or its + entry has a `gap`, or no `evidenceFile` at all → run the + discovered skill/tool live, exactly as before — THEN write the + result back via `contributeGithubEvidence`/ + `contributeLogsEvidence` with your own testRunId as writerId + (Operating Principle 0) so this fills the gap for whoever + reads the file next. + Digest into one block. Record evidenceType in asks_fulfilled (dedupe). gap → emit an `unavailable` block (record in asks_unavailable). NEVER prompt. PRODUCT_BUG in play + no supported PR yet → widen the github hunt this turn. Concatenate per-ask blocks into the next-turn MESSAGE (respect size caps). 4. SUBMIT follow-up on the SAME thread: tfaRcaTurn(testRunId, message, threadId). turns_used += 1. + FAILS ("TFA agent run failed") → resubmit the SAME message on the SAME + thread once (per 4b), still counting as a turn. If THAT resubmit also + fails (two consecutive same-thread failures) → per 4b, if no restart has + happened yet this run: submit a condensed hypothesis as turn 1 of a + BRAND NEW thread (no threadId), capture the new threadId, turns_used += 1, + go to 2. If a restart already happened once and this (the restarted) + thread also hits two consecutive failures → END (PENDING, note + "likely-context-exceeded") — no second restart. 5. TURN-CAP CHECK: if turns_used >= turnCap and still NEEDS_INFO → END (PENDING, "turn-cap"). else → go to 2 with the new result. 6. EMIT the RCA_OUTPUT block from the captured terminal state. @@ -232,9 +539,10 @@ RCA_OUTPUT_END Notes: - `status` is one of exactly three values. `turn-cap`, `soft-pending` (drain - budget spent) and `blocked` all report as `PENDING`; note which in `root_cause`. - A `PENDING` from a *drained* turn should never appear — a drain that lands - re-classifies instead. + budget spent), `blocked`, and `likely-context-exceeded` (two consecutive + same-thread `TFA agent run failed` resubmits, per 4b) all report as + `PENDING`; note which in `root_cause`. A `PENDING` from a *drained* turn + should never appear — a drain that lands re-classifies instead. - `asks_skipped` always includes `test_logs` whenever TFA asked for logs. `asks_fulfilled` **never** includes `test_logs`. - `asks_unavailable` is the evidence-coverage signal the coverage stamp turns @@ -244,6 +552,8 @@ Notes: ## Hard limits +- **Never** treat a `gap`-marked `evidenceFile` entry as coverage — a `gap` + means attempt a live call exactly as if the file didn't have that entry. - **Never** prompt, ask, or wait on a user — the gate is closed; gaps degrade to `unavailable`. - **Never** fulfill or seed a `test_logs` ask — TFA owns logs. - **Never** exceed `turnCap` `tfaRcaTurn` calls in one run. @@ -253,6 +563,10 @@ Notes: - **Never** let drain reads consume the turn cap, and never drain past the `softPendingDrain` budget — a wedged turn must not hang the batch. - **Never** dump raw logs, full diffs, or full file contents into a turn message — digest only. +- **Never** run an unfiltered gather call (a bare `gh api ...` with no `--jq`, + `kubectl get ... -o wide`/`-o yaml` when a narrower `-o custom-columns` + answers the ask) — project to the needed field(s) before the call runs, not + by reading past the noise after. - **Never** write to any repo / cluster / ticket / the run — every action is read-only. - **Never** editorialize a cause — pass TFA's `glimpse` through verbatim. - **Never** blindly inherit a representative's cause for a sibling — confirm against its own logs. diff --git a/bin/cached-exec.mjs b/bin/cached-exec.mjs new file mode 100644 index 0000000..b6ab9fd --- /dev/null +++ b/bin/cached-exec.mjs @@ -0,0 +1,163 @@ +#!/usr/bin/env node +// Run a READ-ONLY command through the build's tool cache, in ONE tool call. +// +// Why a wrapper: a "check cache / run / store" sequence done by hand costs +// three tool calls to save one, which is worse than not caching. This collapses +// it to a single call that behaves exactly like the underlying command — +// same stdout, same exit code — but only actually executes on a miss. +// +// Usage (command is ONE argument, so the caller's own quoting survives): +// node bin/cached-exec.mjs '' +// node bin/cached-exec.mjs - # command on STDIN +// node bin/cached-exec.mjs --stats +// +// Wrap only the expensive fetch and leave filtering to the outer shell: +// node bin/cached-exec.mjs "$B" 3895581484 'gh api repos/o/r/contents/f' | jq -r .content | head -40 +// Two coordinators piping the same fetch through different greps then share +// one cache entry, instead of each paying for the fetch. +// +// TWO GOTCHAS, both hit in real use: +// +// 1. Hit/miss banners go to STDERR, so stdout stays byte-identical to the raw +// command and `| jq` works. But `2>&1 | jq` merges the banner back into +// the pipe and jq dies on it ("Invalid literal at line 1, column 12"). +// Don't redirect stderr into a pipe. If you silence it with `2>/dev/null` +// you also lose the hit/miss signal — so set `TOOLCACHE_LOG=` and +// the banners are teed there too: `grep -c HIT ` still works. +// +// 2. Nested single quotes. A command containing its own `'…'` (typically +// `--jq '.[] | "\(.number)"'`) cannot be passed inside a single-quoted +// argument — the outer shell terminates the string early and the argument +// arrives mangled. Use `-` and pipe the command in on stdin instead: +// printf '%s' 'gh pr list -R o/r --json number --jq ".[].number"' \ +// | node bin/cached-exec.mjs "$B" 3895 - + +import { execFileSync } from "node:child_process"; +import { readFileSync, appendFileSync } from "node:fs"; +import { + toolCacheDirFor, cacheKey, cacheGet, cachePut, cacheStats, isRunnable, tokenize, +} from "../lib/tool-cache.mjs"; + +const [, , buildId, writerOrFlag, commandArg] = process.argv; + +// `-` means the command arrives on stdin, which sidesteps the nested-quoting +// problem entirely (see gotcha 2 above). +let command = commandArg; +if (command === "-") { + try { + command = readFileSync(0, "utf8").trim(); + } catch { + command = ""; + } + if (!command) { + console.error("[tool-cache] '-' given but stdin was empty"); + process.exit(2); + } +} + +if (!buildId || (writerOrFlag !== "--stats" && !command)) { + console.error("usage: cached-exec.mjs ''"); + console.error(" cached-exec.mjs --stats"); + process.exit(2); +} + +const dir = toolCacheDirFor(buildId, process.env.RCA_STATE_DIR ?? ""); + +// Where hit/miss banners go. Default stderr keeps stdout byte-identical to the +// wrapped command. But callers pipe stdout into jq/sed and silence stderr with +// `2>/dev/null` to keep the tool chatter out — which also throws away the +// banner, so the run's own hit-rate becomes unmeasurable. Setting +// TOOLCACHE_LOG= tees banners to a file, letting a caller suppress +// stderr and still count hits afterwards (`grep -c HIT `). +const logPath = process.env.TOOLCACHE_LOG ?? ""; +function banner(line) { + console.error(line); + if (logPath) { + try { + appendFileSync(logPath, line + "\n", { encoding: "utf8", mode: 0o600 }); + } catch { + /* logging must never break the fetch */ + } + } +} + +if (writerOrFlag === "--stats") { + const s = cacheStats(dir); + console.log(JSON.stringify({ cacheDir: dir, ...s }, null, 2)); + process.exit(0); +} + +// Parse into a fetch + filter chain before anything runs. +const gate = isRunnable(command); +if (!gate.ok) { + console.error(`[tool-cache REFUSED] ${gate.reason}`); + console.error(` command: ${command}`); + process.exit(2); +} + +// Key on the FETCH ONLY. Downstream filters are pure text transforms, so two +// agents filtering the same fetch differently share one cached network call. +const key = cacheKey(gate.fetchText); + +// Run one argv with `input` on stdin, no shell. Returns { stdout, exitCode }. +function run(argv, input) { + try { + return { + stdout: execFileSync(argv[0], argv.slice(1), { + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + // Capture stderr rather than let it inherit: execFileSync otherwise + // BOTH inherits and captures, so relaying it ourselves printed + // failures three times. + stdio: [input === undefined ? "ignore" : "pipe", "pipe", "pipe"], + ...(input === undefined ? {} : { input }), + }), + exitCode: 0, + }; + } catch (err) { + if (err.stderr) process.stderr.write(err.stderr.toString()); // the only copy + return { + stdout: (err.stdout ?? "").toString(), + exitCode: typeof err.status === "number" ? err.status : 1, + }; + } +} + +let fetched; +const hit = cacheGet(dir, key); +if (hit) { + banner(`[tool-cache HIT ${key} — captured by ${hit.writerId ?? "?"}, ${hit.bytes}B]`); + fetched = hit.stdout; +} else { + const res = run(gate.fetch, undefined); + fetched = res.stdout; + if (res.exitCode !== 0) { + // Preserve the real behaviour. Deliberately NOT cached — a transient + // failure (rate limit, expired token) must not become a permanent answer. + banner(`[tool-cache MISS ${key} — fetch exited ${res.exitCode}, NOT cached]`); + process.stdout.write(fetched); + process.exit(res.exitCode); + } + if (fetched.trim() === "") { + // An empty result is usually a wrong selector or a silently failed lookup; + // caching it creates a sticky, invisible negative for every later reader. + banner(`[tool-cache MISS ${key} — empty result, NOT cached]`); + } else { + // nowMs is read here, at the process edge — lib/ keeps its no-clock + // discipline so it stays sandbox-safe. + cachePut(dir, key, { command: gate.fetchText, writerId: writerOrFlag, stdout: fetched, exitCode: 0 }, Date.now()); + banner(`[tool-cache MISS ${key} — stored ${fetched.length}B]`); + } +} + +// Apply the filter chain to whatever the fetch produced (cached or fresh). +let out = fetched; +let finalExit = 0; +for (const f of gate.filters) { + const res = run(f, out); + out = res.stdout; + if (res.exitCode !== 0) { finalExit = res.exitCode; break; } +} + +process.stdout.write(out); +process.exit(finalExit); diff --git a/bin/cached-mcp.mjs b/bin/cached-mcp.mjs new file mode 100644 index 0000000..a5074ee --- /dev/null +++ b/bin/cached-mcp.mjs @@ -0,0 +1,133 @@ +#!/usr/bin/env node +// Memo cache for READ-ONLY **MCP** tool calls, sharing the same per-build +// store as `cached-exec.mjs`. +// +// Shell calls can be wrapped transparently (`cached-exec.mjs` runs the command +// for you). MCP calls cannot — only the agent can invoke an MCP tool — so the +// contract here is check-then-call: +// +// 1. get → node bin/cached-mcp.mjs get '' +// exit 0 + result on stdout = HIT, skip the MCP call entirely +// exit 1, empty stdout = MISS, make the MCP call yourself +// 2. put → node bin/cached-mcp.mjs put '' +// (payload on STDIN — pipe the digest you want shared) +// +// WHEN THIS PAYS OFF, and when it does not. A hit replaces one MCP call with +// one cheap local read, so it wins on latency and on tokens whenever the +// cached payload is a digest smaller than the raw response. A miss costs two +// extra calls (the probe + the store), so this is worth it for **expensive, +// broadly-reusable, build-level queries** — a VictoriaLogs sweep, a +// `listTestIds`, a `getFailureLogs` several coordinators would each re-run — +// and NOT worth it for a one-off lookup only this test will ever need. +// +// Never cacheable (refused): `tfaRcaTurn`, `getTfaTurnResult`, +// `triggerRcaReport`. Those are stateful — a turn's status is *expected* to +// change between reads, so serving one from cache is wrong, not just stale. +// Prefer storing a DIGEST rather than a raw payload: the point is to spare the +// next reader the raw rows, not to relay them. + +import { readFileSync, readdirSync, existsSync, appendFileSync } from "node:fs"; +import { join } from "node:path"; +import { + toolCacheDirFor, mcpCacheKey, cacheGet, cachePut, cacheStats, isCacheableMcp, +} from "../lib/tool-cache.mjs"; + +// Same TOOLCACHE_LOG tee as cached-exec, so shell and MCP hits can be counted +// from one file. Previously only shell banners were logged, which made a run's +// combined hit rate impossible to total. +const logPath = process.env.TOOLCACHE_LOG ?? ""; +function banner(line) { + console.error(line); + if (logPath) { + try { appendFileSync(logPath, line + "\n", { encoding: "utf8", mode: 0o600 }); } catch { /* never break the call */ } + } +} + +const [, , buildId, verb, tool, argsJson, writerId] = process.argv; + +if (!buildId || !verb) { + console.error("usage: cached-mcp.mjs get ''"); + console.error(" cached-mcp.mjs put '' # payload on stdin"); + console.error(" cached-mcp.mjs list # what is cached, with exact args to copy"); + console.error(" cached-mcp.mjs stats"); + process.exit(2); +} + +const dir = toolCacheDirFor(buildId, process.env.RCA_STATE_DIR ?? ""); + +if (verb === "stats") { + console.log(JSON.stringify({ cacheDir: dir, ...cacheStats(dir) }, null, 2)); + process.exit(0); +} + +// `list` exists because a HIT requires reproducing the args EXACTLY, and +// canonicalization only normalizes key ORDER, not content. A coordinator that +// guesses the logql/window/limit triple misses — one real run burned four +// probe calls guessing, to save two. Listing what is actually cached turns +// that into a single call: read the available queries, then `get` the one you +// want with its args copied verbatim. +if (verb === "list") { + if (!existsSync(dir)) { console.log("(no cache yet)"); process.exit(0); } + let n = 0; + for (const f of readdirSync(dir).filter((x) => x.endsWith(".json"))) { + let e; try { e = JSON.parse(readFileSync(join(dir, f), "utf8")); } catch { continue; } + if (!/^mcp__/.test(e.command ?? "")) continue; // shell entries live here too + n++; + const sp = e.command.indexOf(" "); + console.log(`\n[${e.key}] ${e.command.slice(0, sp)} (by ${e.writerId ?? "?"}, ${e.bytes}B)`); + console.log(` args: ${e.command.slice(sp + 1)}`); + console.log(` digest: ${String(e.stdout).replace(/\s+/g, " ").slice(0, 150)}…`); + } + if (!n) console.log("(no MCP entries cached — the orchestrator should pre-seed Step 4's queries)"); + process.exit(0); +} + +if (!tool || argsJson === undefined) { + console.error("both and '' are required"); + process.exit(2); +} + +if (!isCacheableMcp(tool)) { + console.error(`[mcp-cache REFUSED] ${tool} is stateful — never cache it; call it directly.`); + process.exit(2); +} + +let args; +try { + args = JSON.parse(argsJson); +} catch (err) { + console.error(`[mcp-cache] argsJson is not valid JSON: ${err.message}`); + process.exit(2); +} + +const key = mcpCacheKey(tool, args); + +if (verb === "get") { + const hit = cacheGet(dir, key); + if (!hit) { + banner(`[mcp-cache MISS ${key} ${tool}] — make the MCP call, then 'put' the digest`); + process.exit(1); + } + banner(`[mcp-cache HIT ${key} ${tool} — captured by ${hit.writerId ?? "?"}, ${hit.bytes}B]`); + process.stdout.write(hit.stdout); + process.exit(0); +} + +if (verb === "put") { + let payload = ""; + try { + payload = readFileSync(0, "utf8"); // stdin + } catch { + payload = ""; + } + if (!payload.trim()) { + console.error("[mcp-cache] refusing to store an empty payload"); + process.exit(2); + } + const rec = cachePut(dir, key, { command: `${tool} ${argsJson}`, writerId, stdout: payload }, Date.now()); + banner(`[mcp-cache STORED ${key} ${tool} — ${rec.bytes}B]`); + process.exit(0); +} + +console.error(`unknown verb: ${verb}`); +process.exit(2); diff --git a/bin/evidence-show.mjs b/bin/evidence-show.mjs new file mode 100644 index 0000000..04f8bed --- /dev/null +++ b/bin/evidence-show.mjs @@ -0,0 +1,99 @@ +#!/usr/bin/env node +// Print the FOLDED evidence view: the orchestrator's base pre-fetch with every +// coordinator's contribution shard merged on top. +// +// Why this exists: coordinators are handed one path — the base file — and +// naturally read it with `cat`/`jq`. That shows base ONLY, so every +// contribution written by a sibling is invisible. A real run hit this: an +// agent reported "the file has 2 repos" when the folded view had 5, including +// the 11-PR observability-api entry a prior coordinator had contributed. The +// shard layout is what makes concurrent write-back safe, so the fix is to give +// the merged view its own command rather than to abandon shards. +// +// Usage: +// node bin/evidence-show.mjs # full folded JSON +// node bin/evidence-show.mjs --summary # one line per repo/workload +// node bin/evidence-show.mjs --repo + +import { readEvidenceFile, readBaseFile, contribDirFor, hasTrustworthyPrList, stalenessOf } from "../lib/evidence-file.mjs"; +import { existsSync, readdirSync } from "node:fs"; + +const [, , filePath, mode, arg] = process.argv; +if (!filePath) { + console.error("usage: evidence-show.mjs [--summary | --repo ]"); + process.exit(2); +} + +const folded = readEvidenceFile(filePath); + +// Warn on EVERY view, not just --summary. A resumed run reuses this file by +// buildId alone, and deployState/PR-window data keeps moving after it was +// written — the same silent-wrong-answer risk we refuse branch names over. +// stderr, so it never pollutes JSON piped into jq. +{ + const s = stalenessOf(filePath, Date.now()); + if (s.stale || !s.known) console.error(`[evidence-show STALE] ${s.note}`); +} + +if (mode === "--repo") { + console.log(JSON.stringify(folded.github?.[arg] ?? null, null, 2)); + process.exit(0); +} + +// `--prs` prints the one table that does the most falsification work per byte: +// mergedAt | #num | title. A coordinator compares mergedAt against the build's +// start_at and disqualifies everything merged after it — no diffs fetched. On +// one real run that removed 11 of 22 candidates before a single `gh pr view`, +// and getting there previously required piping --repo's raw JSON through an +// ad-hoc node one-liner. +if (mode === "--prs") { + const repos = arg ? [arg] : Object.keys(folded.github ?? {}); + for (const repo of repos) { + const e = folded.github?.[repo]; + if (!e) { console.log(`${repo}: (not in evidence file)`); continue; } + const prs = e.prsInWindow ?? []; + const trust = e.prsSearched === true || prs.length > 0 ? "" : " [LIST NOT TRUSTWORTHY — never searched]"; + console.log(`\n${repo} (${prs.length} PR(s))${trust}`); + for (const p of prs.sort((a, b) => String(a.mergedAt).localeCompare(String(b.mergedAt)))) { + console.log(` ${p.mergedAt ?? "?".padEnd(24)} ${String(p.pr).padEnd(7)} ${String(p.title ?? "").slice(0, 88)}`); + } + } + const w = folded.suspectWindow; + if (w?.startedAt) { + console.log(`\nbuild started_at: ${w.startedAt}`); + console.log(" → anything merged AFTER that could not have shipped in this build (window guard)."); + } + process.exit(0); +} + +if (mode !== "--summary") { + console.log(JSON.stringify(folded, null, 2)); + process.exit(0); +} + +const base = readBaseFile(filePath); +const dir = contribDirFor(filePath); +const shards = existsSync(dir) ? readdirSync(dir).filter((f) => f.endsWith(".json")) : []; + +console.log(`build : ${folded.buildId}`); +console.log(`base repos : ${Object.keys(base.github ?? {}).length}`); +console.log(`contribution shards: ${shards.length} (${shards.map((s) => s.replace(".json", "")).join(", ") || "none"})`); +console.log(""); +console.log("github (folded):"); +for (const [repo, e] of Object.entries(folded.github ?? {})) { + const prs = (e.prsInWindow ?? []).length; + const trust = hasTrustworthyPrList(folded, repo) ? "trustworthy" : "PR LIST NOT TRUSTWORTHY (never searched)"; + console.log(` ${repo}: ${prs} PR(s), deployState=${e.deployState ? "yes" : "no"}, gap=${e.gap ?? "none"} — ${trust}`); +} +console.log(""); +console.log("logs (folded):"); +for (const [wl, e] of Object.entries(folded.logs ?? {})) { + const k = e.kubectlSweep?.gap ? "gapped" : e.kubectlSweep ? "present" : "absent"; + const v = e.victorialogs?.gap ? "gapped" : e.victorialogs ? "present" : "absent"; + console.log(` ${wl}: kubectl=${k}, victorialogs=${v}, gap=${e.gap ?? "none"}`); +} +if (folded.coverage?.reposWithUntrustedPrList?.length) { + console.log(""); + console.log(`WARNING untrusted PR lists: ${folded.coverage.reposWithUntrustedPrList.join(", ")}`); + console.log(" an empty prsInWindow here does NOT mean 'no PRs' — search live before concluding."); +} diff --git a/bin/repo-read.mjs b/bin/repo-read.mjs new file mode 100644 index 0000000..9f904b6 --- /dev/null +++ b/bin/repo-read.mjs @@ -0,0 +1,118 @@ +#!/usr/bin/env node +// Read a repo file at a pinned commit, preferring a local clone over the +// network, and falling back to `gh` automatically. +// +// node bin/repo-read.mjs [--fetch] +// +// Measured on this workspace: local `git show` ~37ms vs `gh api` ~1022ms for +// the same file, byte-identical. One targeted `git fetch` (~5s) makes a stale +// clone usable, so the fetch pays for itself after ~6 reads of that repo. +// +// SHA-PINNED ONLY. A branch name is refused: local clones are routinely behind +// (12 commits, on this machine), and reading a branch locally returned +// different bytes than the real head — which for RCA means confidently +// reasoning about code that never shipped. Get the sha from the evidence +// file's `deployState` (branch tip at build start), which Step 4 records. +// +// Generic by construction: the workspace root and shipping branch are INPUTS +// (RCA_WORKSPACE_ROOT / RCA_SHIPPING_BRANCH) supplied by the gate and the +// product's connector skill. This file names no repo, no branch and no path. +// +// Remote results still go through the tool cache, so a repo with no local +// clone degrades to exactly the previous behaviour. + +import { execFileSync } from "node:child_process"; +import { readFileAt, discoverWorkspaceRoot } from "../lib/repo-source.mjs"; +import { toolCacheDirFor, cacheKey, cacheGet, cachePut } from "../lib/tool-cache.mjs"; + +const [, , buildId, writerId, repo, sha, path, ...flags] = process.argv; +if (!buildId || !writerId || !repo || !sha || !path) { + console.error("usage: repo-read.mjs [--fetch]"); + console.error(" sha must be a COMMIT SHA (a branch name is refused — it can read stale code)"); + process.exit(2); +} + +// NO HARDCODED WORKSPACE OR BRANCH. Which repos exist, where they are checked +// out, and what branch ships are facts the CONNECTOR SKILL owns and the gate +// resolves. Baking either in would make the plugin work for exactly one +// product on one machine — the coupling the capability-manifest design exists +// to avoid. +// +// Resolution order, cheapest and most authoritative first: +// 1. the evidence file's `localRepos` — resolved ONCE at the gate, so a +// coordinator does no filesystem probing at all; +// 2. RCA_WORKSPACE_ROOT, if the caller set it; +// 3. a bounded structural guess (this dir, its parent, grandparent), +// accepted only if it actually contains the repo being asked for. +// Anything else: give up and use the network. Guessing harder risks reading +// an unrelated checkout, which is silently wrong rather than merely slow. +let workspaceRoot = process.env.RCA_WORKSPACE_ROOT; +let rootSource = workspaceRoot ? "RCA_WORKSPACE_ROOT" : null; + +// Derived from the buildId we already have, so there is no env var for a +// dispatch prompt to forget; an explicit override still wins. +if (!workspaceRoot) { + try { + const { readEvidenceFile, evidencePathFor } = await import("../lib/evidence-file.mjs"); + const evidencePath = process.env.RCA_EVIDENCE_FILE + || evidencePathFor(buildId, process.env.RCA_STATE_DIR ?? ""); + const lr = readEvidenceFile(evidencePath)?.localRepos; + if (lr?.workspaceRoot) { workspaceRoot = lr.workspaceRoot; rootSource = "evidence-file (resolved at gate)"; } + } catch { /* evidence file optional */ } +} + +if (!workspaceRoot) { + const here = new URL("..", import.meta.url).pathname; + const d = discoverWorkspaceRoot({ repos: [repo], from: here, maxTries: 3 }); + if (d.root) { workspaceRoot = d.root; rootSource = `auto-discovered (${d.tried.length} tr${d.tried.length === 1 ? "y" : "ies"})`; } + else console.error(`[repo-read] no local workspace found — ${d.reason}`); +} +// Only needed to widen a fetch on a miss; a sha-only fetch is attempted when +// absent. Supplied by the connector skill, which knows the shipping branch. +const branch = process.env.RCA_SHIPPING_BRANCH || undefined; +const allowFetch = flags.includes("--fetch"); + +const local = workspaceRoot + ? readFileAt({ repo, sha, path, workspaceRoot, branch, allowFetch }) + : { ok: false, source: "remote-needed", reason: "no local workspace resolved" }; + +if (local.ok) { + console.error(`[repo-read LOCAL ${repo}@${sha.slice(0, 8)} ${local.content.length}B — no network, root via ${rootSource}]`); + process.stdout.write(local.content); + process.exit(0); +} + +// A path genuinely absent at that commit is an answer; don't re-ask the network. +if (local.source === "local") { + console.error(`[repo-read LOCAL ${repo}@${sha.slice(0, 8)}] ${local.reason}`); + process.exit(1); +} + +console.error(`[repo-read -> remote] ${local.reason}`); + +const dir = toolCacheDirFor(buildId, process.env.RCA_STATE_DIR ?? ""); +const cmd = `gh api repos/${repo}/contents/${path}?ref=${sha}`; +const key = cacheKey(cmd); +const hit = cacheGet(dir, key); +if (hit) { + console.error(`[repo-read CACHE HIT ${key} — captured by ${hit.writerId ?? "?"}, ${hit.bytes}B]`); + process.stdout.write(hit.stdout); + process.exit(0); +} + +let out; +try { + const raw = execFileSync("gh", ["api", `repos/${repo}/contents/${path}?ref=${sha}`, "--jq", ".content"], + { encoding: "utf8", maxBuffer: 64 * 1024 * 1024, stdio: ["ignore", "pipe", "pipe"] }); + out = Buffer.from(raw.replace(/\s+/g, ""), "base64").toString("utf8"); +} catch (err) { + if (err.stderr) process.stderr.write(err.stderr.toString()); + console.error(`[repo-read REMOTE failed, NOT cached]`); + process.exit(typeof err.status === "number" ? err.status : 1); +} + +if (out.trim() !== "") { + cachePut(dir, key, { command: cmd, writerId, stdout: out, exitCode: 0 }, Date.now()); + console.error(`[repo-read REMOTE ${out.length}B — cached]`); +} +process.stdout.write(out); diff --git a/config/rca.config.json b/config/rca.config.json index 802f6f4..aea7ace 100644 --- a/config/rca.config.json +++ b/config/rca.config.json @@ -1,15 +1,18 @@ { "$comment": "Central config for the /rca-build RCA harness. All formerly-hardcoded product/infra values live here. No kubectl/chitragupta/bifrost literals — connectors are discovered and probe-validated at the gate into the capability manifest (see skills/rca-build/references/evidence-routing.md). No reportFile: the plugin never writes a local RCA report — the full report lives on the Test Observability UI (triggerRcaReport).", "mcpServerName": "bstack", - "concurrency": 5, + "$concurrencyComment": "Max coordinator subagents run in parallel during Step 5 fan-out. HONORED LITERALLY on the default path (direct Agent-tool dispatch of ai-tfa-coordinator subagents — one message, up to `concurrency` tool-use blocks per batch) and by the sequential harness (lib/loop.mjs). Only a SOFT target on the opt-in Workflow-tool path (workflows/rca-batch.mjs), which the Workflow runtime hard-caps at min(16, cpu cores - 2) regardless of this value — that cap is architectural and cannot be raised from this repo. See SKILL.md Step 5.", + "concurrency": 20, "turnCap": 6, - "turnMessageMaxChars": 5000, + "turnMessageMaxChars": 1000, "pollSoftPendingMs": 90000, "$softPendingDrainComment": "tfaRcaTurn abandons its in-call poll at pollSoftPendingMs (90s) and returns a soft PENDING while the TFA agent keeps working — turns finalizing past 90s are routine. On a soft PENDING the loop READS the same turnId via getTfaTurnResult on this budget before it routes asks or submits anything further; reads do not consume turnCap. Only when the budget is spent does the run end PENDING (pending-resume row).", + "$maxErrorReadsComment": "A soft PENDING is drained on the full budget below, but a HARD read failure (a thrown error, or a result whose status/message says the TFA agent run failed) is a different signal: it will not resolve by asking again. After this many CONSECUTIVE failed reads the drain stops early and the row ends PENDING with a `tfa-error` note, still resumable. A single good read clears the streak. Measured motivation: on one real build, drain reads plus their sleeps were 23% of all coordinator tool calls, and the four tests that wedged this way were the four slowest in the batch.", "softPendingDrain": { "maxWaitMs": 600000, "intervalMs": 5000, - "maxReads": 40 + "maxReads": 40, + "maxErrorReads": 3 }, "reaperHeartbeatTtlSec": 600, "errorSummaryMaxChars": 200, diff --git a/lib/build-cleanup.mjs b/lib/build-cleanup.mjs new file mode 100644 index 0000000..e8faab7 --- /dev/null +++ b/lib/build-cleanup.mjs @@ -0,0 +1,59 @@ +// Deletes ONE build's own temp/registry artifacts once its RCA report has +// generated successfully (skills/rca-build/SKILL.md Step 6, after +// triggerRcaReport succeeds — never before, never on a partial/failed run). +// +// This is deliberately NOT lib/state-dir.mjs's pruneStateDir: that function is +// a periodic, age-based sweep across every build in the shared temp dir, kept +// manual because it can't tell a finished build from an abandoned one and +// deleting a resumable build's state would silently break `pending-resume`. +// This module never faces that ambiguity — it only runs for a build whose +// every CSV row is already terminal and whose report just generated, so there +// is nothing left in this build's own state to resume. `pruneStateDir` still +// exists as the safety net for builds that never reach Step 6 (crashed +// mid-run); wiring that in is a separate, unrelated concern. +// +// Deletes exactly the four artifact families a build can produce, all scoped +// by buildId so a concurrent run over a DIFFERENT build in the same stateDir +// is never touched: +// - rca-state..csv (lib/csv-state.mjs) +// - rca-evidence..json (+ .contrib/) (lib/evidence-file.mjs) +// - rca-toolcache./ (lib/tool-cache.mjs) +// - rca-turn1..json (lib/turn1-registry.mjs) + +import { existsSync, rmSync } from "node:fs"; +import { csvPathFor } from "./csv-state.mjs"; +import { evidencePathFor, contribDirFor } from "./evidence-file.mjs"; +import { toolCacheDirFor } from "./tool-cache.mjs"; +import { turn1PathFor } from "./turn1-registry.mjs"; + +/** + * Delete this build's own CSV, evidence file + contribution shards, tool + * cache, and turn1 registry. Best-effort per path: a missing file is not an + * error (not every build produces a turn1 registry, e.g.), and a delete that + * throws (permissions, vanished mid-sweep) is recorded in `errors` rather than + * aborting the rest of the cleanup. + * + * Returns `{ deleted: [paths], errors: [{path, message}] }`. + */ +export function cleanupBuildArtifacts(buildId, stateDir = "") { + const targets = [ + csvPathFor(buildId, stateDir), + evidencePathFor(buildId, stateDir), + contribDirFor(evidencePathFor(buildId, stateDir)), + toolCacheDirFor(buildId, stateDir), + turn1PathFor(buildId, stateDir), + ]; + + const deleted = []; + const errors = []; + for (const path of targets) { + if (!existsSync(path)) continue; + try { + rmSync(path, { recursive: true, force: true }); + deleted.push(path); + } catch (err) { + errors.push({ path, message: err?.message ?? String(err) }); + } + } + return { deleted, errors }; +} diff --git a/lib/csv-state.mjs b/lib/csv-state.mjs index 152e51b..5d636f5 100644 --- a/lib/csv-state.mjs +++ b/lib/csv-state.mjs @@ -13,7 +13,7 @@ // is sufficient for the in-process 5-concurrent workflow (true multi-process // locking is out of scope). -import { readFileSync, writeFileSync, existsSync, mkdirSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, mkdirSync, chmodSync } from "node:fs"; import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; @@ -52,6 +52,11 @@ export const COLUMNS = [ "possible_fix", "related_prs", "coverage", + // Both are part of the RCA_OUTPUT contract but had no column, so `flip` + // silently discarded them — `view_rca` in particular is the dashboard link + // the whole run exists to produce. + "view_rca", + "turns_used", "confidence", "timestamp", ]; @@ -130,7 +135,25 @@ export function readRows(csvPath) { const text = readFileSync(csvPath, "utf8"); const raw = parseCsv(text).filter((r) => r.some((c) => c.length > 0)); if (raw.length === 0) return []; - const header = raw[0]; + // Normalise the HEADER, not just flip()'s field names. writeRows only ever + // emits COLUMNS, so any header name we fail to recognise here is silently + // dropped the next time the file is written. That is not theoretical: a + // legacy 10-column state file (`test_id,test_name,…`) round-tripped through + // flip() came back with test_id and test_name gone and cluster_id blanked, + // reporting success the whole way. Losing which test a row describes is + // worse than any error we could raise. + const header = raw[0].map((c) => COLUMN_ALIASES.get(c) ?? c); + const unknown = header.filter((c) => c && !COLUMNS.includes(c)); + if (unknown.length) { + // Loud, and it names the columns — a foreign schema means this file was + // written by a different version, and guessing an alignment for it would + // reintroduce exactly the silent corruption above. + throw new Error( + `[csv-state] ${csvPath} has ${unknown.length} unrecognised column(s): ${unknown.join(", ")}. ` + + `This file was written by a different schema version; writing it back would DROP those columns. ` + + `Re-seed the build instead of resuming this file.`, + ); + } return raw.slice(1).map((cells) => { const row = {}; header.forEach((col, idx) => { @@ -140,10 +163,29 @@ export function readRows(csvPath) { }); } +// Owner-only (0700 dir / 0600 file): the state CSV lives in a world-readable +// OS temp dir and records root causes, culprit PRs and evidence digests. export function writeRows(csvPath, rows) { const dir = dirname(csvPath); - if (dir && !existsSync(dir)) mkdirSync(dir, { recursive: true }); - writeFileSync(csvPath, encodeRows(rows), "utf8"); + // `mode` applies on CREATE only — the same trap already fixed for the files + // themselves. A directory made before hardening stays 0755 forever, which on + // a shared machine leaves root causes, culprit PRs and log excerpts readable + // by every local user. Tighten an existing one too. + if (dir) ensureOwnerOnlyDir(dir); + const existed = existsSync(csvPath); + writeFileSync(csvPath, encodeRows(rows), { encoding: "utf8", mode: 0o600 }); + // `mode` applies on create only — tighten a pre-hardening leftover too. + if (existed) chmodSync(csvPath, 0o600); +} + +// Owner-only, on create AND on an existing directory. `mkdirSync`'s `mode` +// applies only when it creates the dir, so one made before this hardening +// landed keeps its 0755 forever — and these artifacts hold root causes, +// culprit PRs and log excerpts in a shared OS temp dir. Found in practice: +// /bstack-rca was drwxr-xr-x with 0600 files inside it. +function ensureOwnerOnlyDir(dir) { + if (!existsSync(dir)) { mkdirSync(dir, { recursive: true, mode: 0o700 }); return; } + try { chmodSync(dir, 0o700); } catch { /* not ours to tighten; leave it */ } } function emptyRow() { @@ -216,22 +258,115 @@ export function heartbeat(csvPath, testRunId, worker, nowMs) { // in-flight claim. `fields` carries any of: rca_done, root_cause, failure_type, // possible_fix, related_prs, threadId, turnId, coverage, confidence, // last_evidence_digest, cluster_id. +// The RCA_OUTPUT contract speaks `RESOLVED | PENDING | failed`, while the CSV +// stores `resolved | blocked | failed | pending-resume`. Callers naturally pass +// the vocabulary their own output block mandates, so accept it and translate +// rather than silently rejecting — a silent `false` here cost a whole batch of +// results, since the row simply stayed `pending` and looked un-run. +const FLIP_ALIASES = new Map([ + ["resolved", "resolved"], + ["pending", RESUMABLE], + ["pending-resume", RESUMABLE], + ["blocked", "blocked"], + ["failed", "failed"], + ["done", "resolved"], +]); + +// Column aliases for the same reason: the output block says `thread_id` and +// `status`, the CSV says `threadId` and `rca_done`. +const COLUMN_ALIASES = new Map([ + ["thread_id", "threadId"], + ["turn_id", "turnId"], + ["status", "rca_done"], + ["test_run_id", "testRunId"], +]); + export function flip(csvPath, testRunId, fields, nowMs) { + // Arity guard. `flip` is positional with csvPath FIRST, and a caller that + // drops it — `flip(testRunId, fields)` — otherwise binds an object to + // testRunId, reads a nonexistent CSV, and gets a bare `false` that is easy + // to mistake for success. Name the mistake precisely instead. + if (typeof csvPath !== "string" || (testRunId !== null && typeof testRunId === "object")) { + console.warn( + "[csv-state] flip called with the wrong arguments. Signature is " + + "flip(csvPath, testRunId, fields, nowMs) — csvPath FIRST, e.g. " + + "flip(csvPathFor(buildId), '3904695279', { status: 'RESOLVED', ... }, Date.now()). " + + `Got csvPath=${JSON.stringify(csvPath)?.slice(0, 60)}, testRunId=${JSON.stringify(testRunId)?.slice(0, 60)}. Row NOT written.`, + ); + return false; + } // Enforce the contract: a flip must name a valid outcome. A partial flip with // a missing/non-terminal rca_done would otherwise clear the claim yet leave the // row `pending` — re-exposing it for a duplicate RCA that clobbers this result. // Reject without mutating so the worker keeps its claim and the bug surfaces. - if (!FLIP_STATES.has(fields?.rca_done)) return false; + const raw = fields?.rca_done ?? fields?.status; + const state = FLIP_ALIASES.get(String(raw ?? "").trim().toLowerCase()); + if (!state) { + // Loud, not silent: the previous bare `false` was indistinguishable from + // success to a caller that didn't check, and results were lost that way. + console.warn( + `[csv-state] flip REJECTED for testRunId=${testRunId}: rca_done=${JSON.stringify(raw)} ` + + `is not one of ${[...new Set(FLIP_ALIASES.values())].join(" | ")} (case-insensitive). Row NOT written.`, + ); + return false; + } const rows = readRows(csvPath); const row = rows.find((r) => String(r.testRunId) === String(testRunId)); - if (!row) return false; - for (const [k, v] of Object.entries(fields)) { + if (!row) { + console.warn(`[csv-state] flip REJECTED: no row for testRunId=${testRunId} in ${csvPath}`); + return false; + } + const dropped = []; + for (const [k0, v] of Object.entries(fields)) { + const k = COLUMN_ALIASES.get(k0) ?? k0; if (COLUMNS.includes(k)) { row[k] = Array.isArray(v) ? v.join("; ") : (v ?? ""); + } else { + dropped.push(k0); } } + row.rca_done = state; // normalized, whatever spelling arrived + if (dropped.length) { + console.warn(`[csv-state] flip ignored unknown field(s) for ${testRunId}: ${dropped.join(", ")}`); + } row.in_flight_worker = ""; row.timestamp = String(nowMs); + + // A `pending-resume` row is a PROMISE that this thread can be picked up + // again, and the resume path keeps that promise by calling + // getTfaTurnResult(testRunId, turnId) BEFORE submitting anything new. TFA + // returns a `turnId` only on a soft-`PENDING` turn — precisely the case that + // produces this state — so a resumable row without one cannot be drained: the + // resume would submit blind on a thread that still has an in-flight turn. + // + // Not an error, because losing the row entirely would be worse than resuming + // imperfectly. But it must be loud: silently un-resumable rows look identical + // to healthy ones in the CSV. + // "A PRODUCT_BUG RCA without a culprit PR is incomplete" is one of the + // plugin's hard rules, and it lived only in the prompt. It has held so far — + // across four builds, 3 such rows carried a PR link and 6 carried an explicit + // "none — searched ", which the rule permits. Zero were blank. But an + // unenforced rule is one distracted turn away from a silent product-bug + // attribution with no evidence trail, and a blank field is indistinguishable + // from a genuine dead end in the CSV. Cheap to guard, so guard it. + // + // A stated "none, searched X" satisfies the rule; only EMPTY does not. + if (/PRODUCT_BUG|application/i.test(String(row.failure_type ?? "")) && + !String(row.related_prs ?? "").trim()) { + console.warn( + `[csv-state] testRunId=${testRunId} is ${row.failure_type} with an EMPTY related_prs — ` + + `record the culprit PR link, or state what was searched and why none was found.`, + ); + } + + if (state === RESUMABLE && !String(row.turnId ?? "").trim()) { + console.warn( + `[csv-state] testRunId=${testRunId} flipped to ${RESUMABLE} with NO turnId — ` + + `resume cannot drain the in-flight turn and will submit blind. ` + + `Capture turnId from the PENDING tfaRcaTurn response and pass it to flip().`, + ); + } + writeRows(csvPath, rows); return true; } diff --git a/lib/evidence-file.mjs b/lib/evidence-file.mjs new file mode 100644 index 0000000..e7e42d5 --- /dev/null +++ b/lib/evidence-file.mjs @@ -0,0 +1,579 @@ +// Build-level evidence pre-fetch artifact (see docs/plan: evidence-file). PR +// windows, deploy state, and app-log sweeps are properties of the BUILD, not +// of any one test — a naive batch re-fetches them once per dispatched +// coordinator. This module persists them to a file ONCE so every +// representative and sibling `ai-tfa-coordinator` dispatch can `Read` the same +// artifact instead of re-running the same `gh`/`kubectl`/`grafana` calls. +// +// Layered under `lib/evidence-cache.mjs`, not merged with it: the cache is an +// in-process, function-scoped Map that dedups compute *within* the +// orchestrator's own Step 4 pass; this module is what makes that result +// visible to OTHER processes (the independently-dispatched coordinator +// subagents, which share no memory with the orchestrator or each other). +// +// Path convention mirrors `lib/csv-state.mjs`'s `csvPathFor` exactly: the +// build id is in the filename (no cross-build collisions) and the default +// directory is OS temp (`/bstack-rca/`), so a build's evidence file +// sits right next to its state CSV. `stateDir` overrides the directory only. +// +// Invariant: this file NEVER carries `test_logs` content. `logs` is keyed by +// *workload* (an infra/pod concept), populated only via the `infra`/`logs` +// capability — TFA remains the sole owner of test-side SDK/driver/session +// logs, which structurally cannot land here. +// +// Timestamps are passed in as `nowMs` (never read from the clock here), same +// discipline as `csv-state.mjs`, so this stays usable from the Workflow-tool +// sandbox (which forbids `Date.now()`). +// +// Write-back, WITHOUT a lock and WITHOUT lost updates — single-writer shards. +// The orchestrator's Step 4 pass is not the only writer: a coordinator that +// had to gather live (a repo/PR/workload the pre-fetch didn't cover, or +// covered only with a summary) should persist what it found so a sibling +// dispatched after it — or another cluster sharing the same repo/workload — +// reads the enriched result instead of re-fetching it. +// +// Naively that means N concurrent coordinators read-modify-writing ONE JSON +// file, which drops updates whenever two writes interleave. Instead of a lock +// (fragile, and `csv-state.mjs` already declares multi-process locking out of +// scope) the layout makes contention structurally impossible: +// +// /bstack-rca/ +// rca-evidence..json <- BASE: only the orchestrator writes it +// rca-evidence..contrib/ +// .json <- one file per coordinator; sole writer +// .json +// +// Every file has exactly ONE writer, so no write can ever clobber another's. +// Reads (`readEvidenceFile`) fold base + every shard into a single view, +// deterministically (shards applied in sorted filename order). This is the +// same "the temp dir is ours, use more of it" trick the CSV path convention +// already leans on. + +// Fold precedence, applied per leaf when base and shards disagree: +// 1. Real evidence beats a recorded gap — a coordinator that actually got +// the data overrides the pre-fetch's "couldn't reach this". +// 2. Among two real values, the later shard wins (sorted order), on the +// assumption a coordinator only writes back something deeper than what +// it read. +// 3. `prsInWindow` is unioned by PR number rather than replaced, so two +// coordinators finding different PRs in the same repo both survive. + +import { readFileSync, writeFileSync, existsSync, mkdirSync, readdirSync, chmodSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { tmpdir } from "node:os"; + +const safeName = (v, fallback) => + String(v ?? "").replace(/[^A-Za-z0-9._-]/g, "_") || fallback; + +/** Canonical evidence-file path for one build's run. Same two invariants as + * `csvPathFor`: build id in the filename; OS temp by default; `stateDir` + * overrides the directory only. */ +export function evidencePathFor(buildId, stateDir = "") { + const safe = safeName(buildId, "unknown-build"); + const dir = stateDir && String(stateDir).trim() !== "" ? String(stateDir) : join(tmpdir(), "bstack-rca"); + return join(dir, `rca-evidence.${safe}.json`); +} + +/** Directory holding this build's per-coordinator contribution shards. Derived + * from the base path so callers only ever have to pass `evidenceFilePath` + * around — one input, no second path to thread through every dispatch. */ +export function contribDirFor(basePath) { + return String(basePath).replace(/\.json$/, "") + ".contrib"; +} + +/** The one file a given writer owns. Exactly one writer per path is the whole + * point — never call this for a writerId that isn't yours. */ +export function contribPathFor(basePath, writerId) { + return join(contribDirFor(basePath), `${safeName(writerId, "unknown-writer")}.json`); +} + +/** Shard docs in deterministic (sorted-filename) order. Missing dir → []. A + * corrupt/half-written shard is skipped rather than throwing: a coordinator + * killed mid-write must not break every subsequent read. */ +function readContribs(basePath) { + const dir = contribDirFor(basePath); + if (!existsSync(dir)) return []; + const out = []; + for (const name of readdirSync(dir).filter((n) => n.endsWith(".json")).sort()) { + try { + out.push(JSON.parse(readFileSync(join(dir, name), "utf8"))); + } catch { + // skip unreadable/partial shard + } + } + return out; +} + +// Owner-only, on create AND on an existing directory. `mkdirSync`'s `mode` +// applies only when it creates the dir, so one made before this hardening +// landed keeps its 0755 forever — and these artifacts hold root causes, +// culprit PRs and log excerpts in a shared OS temp dir. Found in practice: +// /bstack-rca was drwxr-xr-x with 0600 files inside it. +function ensureOwnerOnlyDir(dir) { + if (!existsSync(dir)) { mkdirSync(dir, { recursive: true, mode: 0o700 }); return; } + try { chmodSync(dir, 0o700); } catch { /* not ours to tighten; leave it */ } +} + +export function emptyEvidenceFile(buildId, nowMs) { + return { + buildId: String(buildId ?? ""), + generatedAtMs: nowMs, + baseline: null, + suspectWindow: null, + github: {}, + logs: {}, + // Where each repo can be read locally at its pinned sha, resolved ONCE at + // the gate. Without this every coordinator re-probes the filesystem for + // the workspace and re-checks each commit — pure duplicated setup, which + // is the same waste the evidence file exists to remove for PRs and logs. + localRepos: null, + coverage: { reposCovered: [], reposGapped: [], workloadsCovered: [], workloadsGapped: [] }, + }; +} + +/** The BASE file alone, no shards folded in. Internal to the orchestrator's + * write path: `set*`/`merge*` must read-modify-write base only, or they would + * absorb shard content into base and duplicate it on the next fold. */ +// Marker keys stamped on the BASE file so a raw read announces its own +// incompleteness. +// +// Telling coordinators in the prompt to use `evidence-show` was not enough: +// measured on a real run, 21 of 25 evidence-file reads were raw `cat`/`grep`/ +// `Read` against the base path, and only 4 went through the folded view. A raw +// read shows the orchestrator's base ONLY and silently hides every +// contribution shard — which is precisely the representative-to-sibling +// context the file exists to carry. Three different agents each `cat`-ed the +// same file and each saw a partial picture. +// +// So the file now says so itself, in the first bytes anyone sees. JSON has no +// comments, and these keys are the closest thing: they sort first, they are +// unmissable in a `cat` or a `head`, and they name the exact command to run. +// `_` prefixed and stripped on read, so they never reach the fold logic. +const BASE_MARKERS = { + _READ_ME_FIRST: + "PARTIAL VIEW — this is the orchestrator's BASE file only. Every coordinator's " + + "contribution lives in a separate shard alongside it and is NOT in this file. " + + "Reading this path directly (cat/grep/Read) WILL miss evidence other agents already gathered.", + _USE_INSTEAD: "node /bin/evidence-show.mjs --summary (also --prs, --repo )", + _WHY: "Only evidence-show folds base + all shards into the real view. A raw read has cost a real run duplicated work.", +}; + +/** Strip the marker keys — they are documentation for humans and agents, never + * data. Applied on every read so nothing downstream has to know about them. */ +function stripMarkers(doc) { + if (!doc || typeof doc !== "object") return doc; + for (const k of Object.keys(BASE_MARKERS)) delete doc[k]; + return doc; +} + +export function readBaseFile(filePath) { + if (!existsSync(filePath)) return emptyEvidenceFile("unknown-build", 0); + try { + return stripMarkers(JSON.parse(readFileSync(filePath, "utf8"))); + } catch { + return emptyEvidenceFile("unknown-build", 0); + } +} + +// A {block, gap} leaf: real evidence beats a gap; between two real values the +// later (shard) one wins. `undefined`/`null` incoming never overwrites. +function pickLeaf(base, incoming) { + if (incoming == null) return base ?? null; + if (base == null) return incoming; + if (!incoming.gap) return incoming; + if (!base.gap) return base; + return incoming; +} + +// Dedupe key for a PR entry. Union-by-number is right ONLY when a number is +// present: `String(undefined)` is the constant "undefined", so every +// numberless PR collides on one key and the list silently collapses to the +// last one. Observed live — a coordinator wrote back a 6-PR window and the +// file kept 1, with `pr: undefined`, while still reporting the search as +// trustworthy. Fall back to a content key so unnumbered entries survive +// distinctly, and never treat two unknowns as the same PR. +function prKey(pr, index) { + const n = pr?.pr; + if (n !== undefined && n !== null && String(n).trim() !== "" && String(n) !== "undefined") { + return `#${String(n).replace(/^#/, "")}`; + } + const t = String(pr?.title ?? "").trim(); + const u = String(pr?.url ?? pr?.link ?? "").trim(); + return u ? `url:${u}` : t ? `title:${t}` : `anon:${index}`; +} + +function foldGithub(target, repo, entry) { + const cur = target[repo] ?? { deployState: null, prsInWindow: [], gap: null }; + const next = { + deployState: pickLeaf(cur.deployState, entry.deployState), + prsInWindow: cur.prsInWindow ?? [], + // Sticky: once ANY writer has genuinely run the PR search, the entry stays + // trustworthy — a later contributor that didn't search must not silently + // downgrade it back to "unknown". + prsSearched: cur.prsSearched === true || entry.prsSearched === true, + gap: cur.gap ?? null, + }; + if (Array.isArray(entry.prsInWindow)) { + const byPr = new Map((next.prsInWindow ?? []).map((p, i) => [prKey(p, i), p])); + entry.prsInWindow.forEach((pr, i) => byPr.set(prKey(pr, `in-${i}`), pr)); + next.prsInWindow = [...byPr.values()]; + } + // A contributor supplying real content clears the pre-fetch's gap. + if (entry.gap === null || entry.gap === undefined) { + if (entry.deployState || Array.isArray(entry.prsInWindow)) next.gap = null; + } else if (!next.deployState && (next.prsInWindow ?? []).length === 0) { + next.gap = entry.gap; + } + target[repo] = next; +} + +function foldLogs(target, workload, entry) { + const cur = target[workload] ?? { clusterIds: [], kubectlSweep: null, victorialogs: null, gap: null }; + const next = { + clusterIds: [...new Set([...(cur.clusterIds ?? []), ...(entry.clusterIds ?? [])])], + kubectlSweep: pickLeaf(cur.kubectlSweep, entry.kubectlSweep), + victorialogs: pickLeaf(cur.victorialogs, entry.victorialogs), + gap: cur.gap ?? null, + }; + if (entry.gap === null || entry.gap === undefined) { + if (entry.kubectlSweep || entry.victorialogs) next.gap = null; + } else if (!next.kubectlSweep && !next.victorialogs) { + next.gap = entry.gap; + } + target[workload] = next; +} + +/** The full view every CONSUMER should read: the orchestrator's base pre-fetch + * with every coordinator's contribution shard folded on top, deterministically. + * Never throws on a missing base or a corrupt shard — an absent/partial result + * just means those asks fall back to a live gather, which is the whole + * degradation contract. */ +export function readEvidenceFile(filePath) { + const doc = readBaseFile(filePath); + for (const shard of readContribs(filePath)) { + for (const [repo, entry] of Object.entries(shard.github ?? {})) foldGithub(doc.github, repo, entry); + for (const [wl, entry] of Object.entries(shard.logs ?? {})) foldLogs(doc.logs, wl, entry); + if (shard.generatedAtMs > (doc.generatedAtMs ?? 0)) doc.generatedAtMs = shard.generatedAtMs; + } + return doc; +} + +// Owner-only (0700 dir / 0600 file): this sits in a world-readable OS temp dir +// and carries private-repo PR detail and app-log digests. +export function writeEvidenceFile(filePath, doc) { + const dir = dirname(filePath); + // `mode` applies on CREATE only — the same trap already fixed for the files + // themselves. A directory made before hardening stays 0755 forever, which on + // a shared machine leaves root causes, culprit PRs and log excerpts readable + // by every local user. Tighten an existing one too. + if (dir) ensureOwnerOnlyDir(dir); + const existed = existsSync(filePath); + // Markers first, so `head` and any truncated preview show them before data. + const stamped = { ...BASE_MARKERS, ...stripMarkers({ ...doc }) }; + writeFileSync(filePath, JSON.stringify(stamped, null, 2), { encoding: "utf8", mode: 0o600 }); + // `mode` is only honoured when the file is CREATED. A file left over from a + // run that predates this hardening would otherwise keep its old 0644 + // forever, so tighten it explicitly on overwrite too. + if (existed) chmodSync(filePath, 0o600); +} + +function loadOrInit(filePath, nowMs) { + if (!existsSync(filePath)) return emptyEvidenceFile("unknown-build", nowMs); + return readBaseFile(filePath); +} + +/** Idempotent: creates the file with the given `buildId` if it doesn't exist + * yet, otherwise leaves an existing file untouched (never clobbers prior + * writes on a resume). Call this FIRST, before any `set*` call, so `buildId` + * is recorded correctly — the `set*` functions below fall back to + * `"unknown-build"` only as a safety net if called without this. */ +export function initEvidenceFile(filePath, buildId, nowMs) { + if (existsSync(filePath)) return readBaseFile(filePath); + const doc = emptyEvidenceFile(buildId, nowMs); + writeEvidenceFile(filePath, doc); + return doc; +} + +/** + * Persist the once-resolved local-repo map (from `repo-source.mjs`'s + * `discoverWorkspaceRoot` + `resolveLocalRepos`). Shape: + * `{ workspaceRoot, repos: { "org/repo": {dir, sha, usable, reason} } }`. + * + * A coordinator reads this and immediately knows, per repo, whether to use a + * local sha-pinned read or go to the network — with no filesystem probing of + * its own. `workspaceRoot: null` is a legitimate, useful answer: it means + * discovery ran and failed, so nobody should try again. + */ +export function setLocalRepos(filePath, localRepos, nowMs) { + const doc = loadOrInit(filePath, nowMs); + doc.localRepos = localRepos; + doc.generatedAtMs = nowMs; + writeEvidenceFile(filePath, doc); + return doc.localRepos; +} + +/** Records the diff/PR-window baseline once, at the start of the Step 4 pass. + * `baseline` is `resolveBaseline(...)`'s return value from `evidence-cache.mjs` + * (`{ref, isFallback}`); `suspectWindow` is whatever shape the active connector + * skill uses to describe the window (e.g. `{reposRequested, startedAt}`). */ +export function setBaseline(filePath, baseline, suspectWindow, nowMs) { + const doc = loadOrInit(filePath, nowMs); + doc.baseline = baseline; + doc.suspectWindow = suspectWindow; + doc.generatedAtMs = nowMs; + writeEvidenceFile(filePath, doc); + return doc; +} + +/** Read-modify-write merge into `doc.github[repo]`. `entry` shape: + * `{ deployState: {block, gap}, prsInWindow: [{pr, files, block, verdict}], + * gap }` — `gap` (top-level, on the repo entry) is what `recomputeCoverage` + * checks; a repo present with a non-null `gap` is NOT counted as covered. + * Only ever touches this one repo's key — every other repo/workload already + * in the file is untouched. */ +export function setGithubEvidence(filePath, repo, entry, nowMs) { + const doc = loadOrInit(filePath, nowMs); + doc.github[repo] = entry; + doc.generatedAtMs = nowMs; + writeEvidenceFile(filePath, doc); + return doc; +} + +/** Read-modify-write merge into `doc.logs[workload]`. `entry` shape: + * `{ clusterIds, kubectlSweep: {block, gap}, victorialogs: {block, gap}, gap }`. + * Same no-clobber guarantee as `setGithubEvidence`, keyed by workload instead + * of repo. */ +export function setLogsEvidence(filePath, workload, entry, nowMs) { + const doc = loadOrInit(filePath, nowMs); + doc.logs[workload] = entry; + doc.generatedAtMs = nowMs; + writeEvidenceFile(filePath, doc); + return doc; +} + +// ---- coordinator write-back: own-shard only, never the base file ---------- +// +// `writerId` must be unique per concurrent writer — the dispatched +// coordinator's `testRunId` is the natural choice (one coordinator per test). +// Because a writer only ever opens its OWN shard, two coordinators writing at +// the same instant touch different files and neither can lose the other's +// update. Reads fold every shard back together (`readEvidenceFile`). + +function loadOwnShard(basePath, writerId, nowMs) { + const p = contribPathFor(basePath, writerId); + if (!existsSync(p)) { + return { path: p, doc: { writerId: String(writerId), generatedAtMs: nowMs, github: {}, logs: {} } }; + } + try { + return { path: p, doc: JSON.parse(readFileSync(p, "utf8")) }; + } catch { + return { path: p, doc: { writerId: String(writerId), generatedAtMs: nowMs, github: {}, logs: {} } }; + } +} + +function writeShard(path, doc) { + const dir = dirname(path); + // `mode` applies on CREATE only — the same trap already fixed for the files + // themselves. A directory made before hardening stays 0755 forever, which on + // a shared machine leaves root causes, culprit PRs and log excerpts readable + // by every local user. Tighten an existing one too. + if (dir) ensureOwnerOnlyDir(dir); + writeFileSync(path, JSON.stringify(doc, null, 2), { encoding: "utf8", mode: 0o600 }); +} + +/** Contribute what THIS coordinator gathered live for a repo — a deeper + * `deployState` (e.g. the full diff, not just a summary), and/or PRs to fold + * into `prsInWindow` (deduped by `pr`). `patch = { deployState?, prsInWindow?, + * gap? }`; omit a field to leave it untouched. Writes only this writer's + * shard, so it can never clobber another coordinator's contribution or the + * orchestrator's base pre-fetch. */ +export function contributeGithubEvidence(basePath, writerId, repo, patch, nowMs) { + const { path, doc } = loadOwnShard(basePath, writerId, nowMs); + const entry = doc.github[repo] ?? { deployState: null, prsInWindow: [], gap: null }; + if (patch.deployState !== undefined) entry.deployState = patch.deployState; + if (Array.isArray(patch.prsInWindow)) { + const byPr = new Map((entry.prsInWindow ?? []).map((p, i) => [prKey(p, i), p])); + patch.prsInWindow.forEach((pr, i) => byPr.set(prKey(pr, `in-${i}`), pr)); + entry.prsInWindow = [...byPr.values()]; + // Contributing a list — even an empty one — means you actually ran the + // search, so record that. Pass `prsSearched: false` explicitly to opt out. + entry.prsSearched = patch.prsSearched !== false; + } + if (patch.prsSearched !== undefined) entry.prsSearched = patch.prsSearched; + if (patch.gap !== undefined) entry.gap = patch.gap; + doc.github[repo] = entry; + doc.generatedAtMs = nowMs; + writeShard(path, doc); + return entry; +} + +/** Contribute what THIS coordinator gathered live for a workload's app-logs. + * Same single-writer-shard discipline as `contributeGithubEvidence`; + * `clusterIds` is unioned rather than replaced. */ +export function contributeLogsEvidence(basePath, writerId, workload, patch, nowMs) { + const { path, doc } = loadOwnShard(basePath, writerId, nowMs); + const entry = doc.logs[workload] ?? { clusterIds: [], kubectlSweep: null, victorialogs: null, gap: null }; + if (patch.kubectlSweep !== undefined) entry.kubectlSweep = patch.kubectlSweep; + if (patch.victorialogs !== undefined) entry.victorialogs = patch.victorialogs; + if (Array.isArray(patch.clusterIds)) { + entry.clusterIds = [...new Set([...(entry.clusterIds ?? []), ...patch.clusterIds])]; + } + if (patch.gap !== undefined) entry.gap = patch.gap; + doc.logs[workload] = entry; + doc.generatedAtMs = nowMs; + writeShard(path, doc); + return entry; +} + +// A requested item is "covered" only if it is present AND its own `gap` field +// is falsy. Presence with a `gap` is a recorded, deliberate miss — not +// coverage — so a coordinator (or this function) never mistakes "we looked +// and couldn't get it" for "we have it." +// +// For a github entry there is a further trap, hit for real in testing: an +// empty `prsInWindow` is byte-identical whether the PR search RAN and found +// nothing, or was never populated at all. A coordinator trusting the former +// reads "no PRs in window" and concludes "no culprit PR" — confidently wrong. +// (Observed: a file asserting 0 PRs for a repo that actually had 21, because +// the loader's search silently returned empty.) So an empty `prsInWindow` +// only counts as coverage when `prsSearched === true` explicitly records that +// the search was really performed. +// Kept deliberately at the REPO level: an entry with deploy state but no PR +// search is still real coverage of that repo. PR-list trustworthiness is a +// narrower question, reported separately as `reposWithUntrustedPrList` so it +// is visible without distorting covered/gapped. +function isCovered(doc, section, key) { + const entry = doc[section]?.[key]; + return Boolean(entry) && !entry.gap; +} + +/** True when this repo entry can be trusted to answer "which PRs were in the + * window" — i.e. it either lists PRs, or explicitly records that the search + * ran and legitimately found none. Coordinators should call this before + * concluding "no culprit PR" from the pre-fetch. */ +export function hasTrustworthyPrList(doc, repo) { + const entry = doc?.github?.[repo]; + if (!entry || entry.gap) return false; + return (entry.prsInWindow ?? []).length > 0 || entry.prsSearched === true; +} + +/** Derives `doc.coverage` from exactly which requested repos/workloads have a + * gap-free entry, and persists it. `requested = {repos:[...], workloads:[...]}` + * — normally the Gate Part A scope-probe-validated repo list and the union of + * workloads every cluster's representative implicates (see SKILL.md Step 4). */ +export function recomputeCoverage(filePath, requested, nowMs) { + // Coverage is judged against the FOLDED view — a gap the orchestrator + // recorded but a coordinator later filled is genuinely covered now — while + // the result is persisted to base, which the orchestrator solely owns. + const folded = readEvidenceFile(filePath); + const doc = loadOrInit(filePath, nowMs); + const repos = requested?.repos ?? []; + const workloads = requested?.workloads ?? []; + doc.coverage = { + reposCovered: repos.filter((r) => isCovered(folded, "github", r)), + reposGapped: repos.filter((r) => !isCovered(folded, "github", r)), + workloadsCovered: workloads.filter((w) => isCovered(folded, "logs", w)), + workloadsGapped: workloads.filter((w) => !isCovered(folded, "logs", w)), + // Repos whose PR list must NOT be read as "no PRs in window": the list is + // empty and nothing recorded that a search actually ran. Surfacing this + // separately keeps a coordinator from concluding "no culprit PR" off an + // array that was simply never filled in. + reposWithUntrustedPrList: repos.filter( + (r) => isCovered(folded, "github", r) && !hasTrustworthyPrList(folded, r), + ), + }; + doc.generatedAtMs = nowMs; + writeEvidenceFile(filePath, doc); + return doc.coverage; +} + +/** + * How stale is this pre-fetch, and is it still safe to trust? + * + * We are careful never to read a repo at a branch name because a stale clone + * yields a confident wrong answer — but the evidence file had exactly the same + * exposure and no guard at all. It is keyed only by `buildId`, so a + * `pending-resume` row hours or days later silently reuses the original + * `deployState` and PR window. Those describe "what was deployed and what + * merged around the build", and both keep moving after the pre-fetch is + * written. Reusing them blind is the same failure mode, just slower to notice. + * + * This does not expire anything — the file stays usable, because stale + * build-level context is still far better than none and the failure window + * itself never moves. It returns a signal the caller can surface, so a + * coordinator re-verifies a suspect PR instead of trusting a day-old list. + * + * `maxFreshMs` defaults to 6h: comfortably longer than any normal batch run + * (minutes), short enough that an overnight resume is flagged. + */ +export function stalenessOf(filePath, nowMs, maxFreshMs = 6 * 60 * 60 * 1000) { + const doc = readEvidenceFile(filePath); + const generatedAtMs = doc?.generatedAtMs ?? 0; + if (!generatedAtMs) { + return { known: false, stale: false, ageMs: null, note: "no generatedAtMs recorded — age unknown" }; + } + // A timestamp in the FUTURE must never read as "fresh". Clamping the age to + // zero is the tempting one-liner and it fails in the worst direction: clock + // skew between the gate host and a coordinator, or a hand-seeded timestamp, + // would silently certify arbitrarily old evidence as current. We can't tell + // the age, so say so rather than guess in the reassuring direction. + if (generatedAtMs > nowMs) { + return { + known: false, + stale: true, + ageMs: null, + generatedAtMs, + note: `generatedAtMs is ${Math.round((generatedAtMs - nowMs) / 60000)}m in the future (clock skew or a seeded timestamp) — age cannot be trusted; re-verify any PR you are about to name as the cause.`, + }; + } + const ageMs = nowMs - generatedAtMs; + const stale = ageMs > maxFreshMs; + const mins = Math.round(ageMs / 60000); + return { + known: true, + stale, + ageMs, + generatedAtMs, + note: stale + ? `pre-fetch is ${mins}m old (> ${Math.round(maxFreshMs / 60000)}m): deployState and the PR window may have moved since. Still usable — the failure window is fixed — but re-verify any PR you are about to name as the cause.` + : `pre-fetch is ${mins}m old — fresh`, + }; +} + +/** + * The build-time commit sha per repo, as a structured map — the input + * `resolveLocalRepos` needs for its `pins`. + * + * Step 4 SHOULD set `deployState.sha` explicitly. It historically didn't: the + * sha lived only in the prose `summary` ("Branch tip on at build + * start = cd88535b (deploy proxy)"), so the one consumer that needs it + * structurally had to regex English, and got an empty map when the wording + * drifted. That silently downgraded every local read to a network call while + * looking like it worked. + * + * So: prefer the explicit field, fall back to parsing the summary, and report + * which happened so a caller can tell "no sha recorded" from "sha recovered + * from prose". A bare 7-40 hex word is NOT enough on its own — timestamps and + * image tags match that too — so the fallback anchors on an `=`/`:` after a + * build-start phrase. + */ +export function deployShas(filePathOrDoc) { + const doc = typeof filePathOrDoc === "string" ? readEvidenceFile(filePathOrDoc) : filePathOrDoc; + const pins = {}; + const source = {}; + for (const [repo, entry] of Object.entries(doc?.github ?? {})) { + const ds = entry?.deployState; + if (!ds) continue; + if (typeof ds.sha === "string" && /^[0-9a-f]{7,40}$/i.test(ds.sha)) { + pins[repo] = ds.sha; + source[repo] = "field"; + continue; + } + const m = String(ds.summary ?? "").match(/at build start\s*[=:]\s*([0-9a-f]{7,40})\b/i); + if (m) { + pins[repo] = m[1]; + source[repo] = "parsed-from-summary"; + } + } + return { pins, source }; +} diff --git a/lib/loop.mjs b/lib/loop.mjs index d4334f4..e0ef66b 100644 --- a/lib/loop.mjs +++ b/lib/loop.mjs @@ -32,7 +32,7 @@ import { routeAsks } from "./routing.mjs"; // Drain budget for one soft-PENDING. Bounded so a wedged turn can never hang the // batch — on exhaustion the loop still ends `PENDING` (resumable via the CSV's // `pending-resume` row), which is the old behaviour as a floor, not a default. -const DEFAULT_DRAIN = { maxWaitMs: 600_000, intervalMs: 5_000, maxReads: 40 }; +const DEFAULT_DRAIN = { maxWaitMs: 600_000, intervalMs: 5_000, maxReads: 40, maxErrorReads: 3 }; const defaultSleep = (ms) => new Promise((r) => setTimeout(r, ms)); @@ -40,6 +40,30 @@ const defaultSleep = (ms) => new Promise((r) => setTimeout(r, ms)); const isAgentStatus = (s) => s === "RESOLVED" || s === "NEEDS_INFO" || s === "BLOCKED"; +/** + * Distinguish "TFA is still thinking" from "this read HARD-FAILED". + * + * These deserve opposite responses and the original drain conflated them: + * a `PENDING` should be waited out on the full budget, but a server-side + * `TFA agent run failed` will keep failing, and patiently re-reading it burns + * the entire 40-read / 10-minute budget to learn nothing. Measured on one real + * build: drain reads + their sleeps were 23% of ALL coordinator tool calls, + * and the four agents that wedged this way were the four slowest in the batch. + * + * Only explicit failure signals count — an unrecognised-but-parseable turn is + * treated as "still working", so a new upstream status can never be + * misclassified as an error and cut the drain short. + */ +function isErrorRead(turn, threw) { + if (threw) return true; + if (turn == null) return true; + if (typeof turn === "string") return /\b(fail(ed|ure)?|error)\b/i.test(turn); + if (turn.error) return true; + if (typeof turn.status === "string" && /^(ERROR|FAILED)$/i.test(turn.status)) return true; + if (typeof turn.message === "string" && /\b(fail(ed|ure)?|error)\b/i.test(turn.message)) return true; + return false; +} + function unavailableBlock(gap) { const what = gap?.ask?.what ?? ""; return [ @@ -56,32 +80,47 @@ function unavailableBlock(gap) { // not a new turn. `readTurn` is read-only and side-effect free, so the only // budget that applies is wall clock / read count. // -// Returns { turn, reads }: `turn` is the landed agent turn, or null if the drain -// budget ran out (caller then ends PENDING, resumable). +// Returns { turn, reads, reason }: `turn` is the landed agent turn, or null if +// the drain gave up — `reason` is `landed` | `tfa-error` | `budget-spent` | +// `not-drainable`, which the caller surfaces in the RCA_OUTPUT note so a human +// can tell "TFA was slow" apart from "TFA broke". async function drainSoftPending({ testRunId, pending, readTurn, sleep, drain }) { - const { maxWaitMs, intervalMs, maxReads } = { ...DEFAULT_DRAIN, ...(drain ?? {}) }; + const { maxWaitMs, intervalMs, maxReads, maxErrorReads } = { ...DEFAULT_DRAIN, ...(drain ?? {}) }; const turnId = pending.turnId; let reads = 0; // No turnId → nothing addressable to read; no readTurn → client lacks the // getTfaTurnResult tool. Either way fall back to reporting it resumable. - if (!turnId || typeof readTurn !== "function") return { turn: null, reads }; + if (!turnId || typeof readTurn !== "function") return { turn: null, reads, reason: "not-drainable" }; const started = Date.now(); + let consecutiveErrors = 0; while (reads < maxReads && Date.now() - started < maxWaitMs) { await sleep(intervalMs); reads++; let turn; + let threw = false; try { turn = await readTurn({ testRunId, turnId }); } catch { - // A failed read is not a verdict — keep reading until the budget is spent. + threw = true; + } + + if (isErrorRead(turn, threw)) { + // Hard failure. Allow a couple of retries for a genuine blip, then stop: + // a wedged turn will not un-wedge by being asked the same question 37 + // more times, and the row stays resumable either way. + if (++consecutiveErrors >= maxErrorReads) { + return { turn: null, reads, reason: "tfa-error" }; + } continue; } - if (isAgentStatus(turn?.status)) return { turn, reads }; + + consecutiveErrors = 0; // a good read clears the streak + if (isAgentStatus(turn?.status)) return { turn, reads, reason: "landed" }; // still PENDING → the agent is working; read again. } - return { turn: null, reads }; + return { turn: null, reads, reason: "budget-spent" }; } // runRcaLoop drives one test to a terminal RCA_OUTPUT object. @@ -89,6 +128,15 @@ async function drainSoftPending({ testRunId, pending, readTurn, sleep, drain }) // submit({ testRunId, message, threadId, turnId }) → Promise (tfaRcaTurn shape) // readTurn({ testRunId, turnId }) → Promise (getTfaTurnResult shape) // gather(routedGatherEntry) → Promise (one digest block) +// turn1Result: { threadId, asks } — a Step 4b pre-dispatch (SKILL.md Step 4b, +// lib/turn1-registry.mjs) already submitted turn 1 for this representative +// and it landed NEEDS_INFO. When present, turn 1 is NEVER submitted again — +// the loop starts already at the ROUTE step with this thread's asks, same +// as `agents/ai-tfa-coordinator.md`'s `turn1_result` input. A pre-dispatch +// that landed PENDING instead uses the existing `resume` convention +// (thread the drained turnId in via the caller's own resume handling) — +// it needs no special case here, since draining a soft-PENDING and then +// re-classifying is exactly what this loop already does. export async function runRcaLoop({ testRunId, firstMessage = "", @@ -100,6 +148,7 @@ export async function runRcaLoop({ turnCap = config?.turnCap ?? 6, drain = config?.softPendingDrain, sleep = defaultSleep, + turn1Result, }) { if (testRunId == null || Number.isNaN(Number(testRunId))) { return { @@ -142,7 +191,14 @@ export async function runRcaLoop({ while (true) { turns++; - let turn = await submit({ testRunId, message, threadId, turnId }); + // Step 4b already ran turn 1 for this representative and it landed + // NEEDS_INFO — treat it as this iteration's result instead of resubmitting + // message 1. Only applies on the very first pass; every later iteration + // submits normally regardless of what turn1Result held. + let turn = + turns === 1 && turn1Result?.threadId + ? { status: "NEEDS_INFO", threadId: turn1Result.threadId, asks: turn1Result.asks ?? [] } + : await submit({ testRunId, message, threadId, turnId }); threadId = turn.threadId ?? threadId; // Soft-PENDING → the in-call poll capped out, not a verdict. Read the SAME @@ -157,11 +213,13 @@ export async function runRcaLoop({ drain, }); if (!drained.turn) { - return out( - "PENDING", - turn, - `soft-pending: still working after ${drained.reads} read(s)`, - ); + const note = + drained.reason === "tfa-error" + ? `tfa-error: read failed ${drained.reads} time(s) — stopped early, row stays resumable` + : drained.reason === "not-drainable" + ? `soft-pending: no turnId or no getTfaTurnResult tool` + : `soft-pending: still working after ${drained.reads} read(s)`; + return out("PENDING", turn, note); } turn = drained.turn; threadId = turn.threadId ?? threadId; @@ -184,12 +242,16 @@ export async function runRcaLoop({ // Route + fulfill. Gaps degrade to `unavailable` — never a user prompt. const buckets = routeAsks(turn.asks ?? [], config, manifest); - const blocks = []; for (const s of buckets.skip) skipped.add(s.evidenceType); - for (const g of buckets.gather) { - blocks.push(await gather(g)); - fulfilled.add(g.evidenceType); - } + // Independent asks: routeAsk/routeAsks (lib/routing.mjs) do pure per-ask + // classification with no cross-ask state, and "high -> medium -> low" only + // orders the assembled message, never a data dependency between one ask's + // gather and another's. So fetch them concurrently instead of one + // round-trip at a time — Promise.all preserves buckets.gather's priority + // order in the result, so message order is unchanged. + const gathered = await Promise.all(buckets.gather.map((g) => gather(g))); + buckets.gather.forEach((g) => fulfilled.add(g.evidenceType)); + const blocks = [...gathered]; for (const gap of buckets.gap) { unavailable.add(gap.evidenceType); blocks.push(unavailableBlock(gap)); diff --git a/lib/repo-source.mjs b/lib/repo-source.mjs new file mode 100644 index 0000000..865430e --- /dev/null +++ b/lib/repo-source.mjs @@ -0,0 +1,173 @@ +// Read repo files from a LOCAL clone when one is available, instead of paying +// a network round-trip per file. +// +// Measured: `gh api .../contents/?ref=` ~1022ms; the same read as +// `git show :` from a local clone ~37ms — 27x faster, and +// byte-identical. Across three real runs, file CONTENTS were 126 of 407 gh +// calls (31%) and commit history another 48 (12%), so this is the largest +// remaining slice of github traffic. +// +// THE CORRECTNESS RULE: ALWAYS PIN TO A COMMIT SHA, NEVER A BRANCH NAME. +// +// This is not pedantry — it is the whole reason this module needs care. A +// developer's clone is usually stale: measured on this workspace, +// the shipping branch's remote-tracking ref was 12 commits behind, and reading +// `testPlan.js` from the local branch returned 281,061 bytes where the real +// branch head had 282,315. For RCA that is catastrophic in a quiet way: we +// reason about *what changed in a window*, so silently reading different code +// yields a confident wrong answer. Pinned to the build-time SHA the content is +// byte-identical to GitHub, and staleness stops mattering — a commit either +// exists locally or it does not, and we can tell which. + +import { execFileSync } from "node:child_process"; +import { existsSync } from "node:fs"; +import { join } from "node:path"; + +const SHA = /^[0-9a-f]{7,40}$/i; + +function git(dir, args) { + return execFileSync("git", ["-C", dir, ...args], { + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + stdio: ["ignore", "pipe", "pipe"], + }); +} + +/** Local clone path for `org/repo`, if one exists under `workspaceRoot`. + * Matches on the bare repo name, which is how these workspaces are laid out. */ +export function localCloneFor(repo, workspaceRoot) { + const name = String(repo).split("/").pop(); + const dir = join(workspaceRoot, name); + return existsSync(join(dir, ".git")) ? dir : null; +} + +/** + * Find the directory holding the local clones, WITHOUT hardcoding a path. + * + * `repos` is the gate's validated repo list, and it is what makes this + * generic: a candidate only wins if it actually contains one of the repos + * THIS run cares about. No repo name, product or path is baked in — a + * different product with a different checkout layout resolves by the same + * rule. + * + * Bounded on purpose: an explicit override, then at most `maxTries` + * structural guesses. Searching the filesystem for a plausible-looking + * directory would risk picking a stale or unrelated checkout, and a wrong + * workspace silently yields the wrong source code — the same class of failure + * as reading a stale branch. Finding nothing is a fine answer; the caller + * falls back to the network. + * + * Returns `{ root, matched, tried }`, or `{ root: null, tried }`. + */ +export function discoverWorkspaceRoot({ repos = [], explicit, from, maxTries = 3 } = {}) { + const verify = (dir) => { + if (!dir || !existsSync(dir)) return null; + const hit = repos.find((r) => localCloneFor(r, dir)); + return hit ? { root: dir, matched: hit } : null; + }; + + // An explicit value is authoritative and not counted as a guess — but it is + // still verified, so a stale env var fails loudly instead of quietly. + if (explicit) { + const ok = verify(explicit); + return ok ? { ...ok, tried: [explicit] } : { root: null, tried: [explicit], reason: `RCA_WORKSPACE_ROOT=${explicit} contains none of the validated repos` }; + } + + // Structural guesses only, in decreasing confidence. `from` is typically the + // plugin's own directory, which usually sits inside the workspace. + const base = from ?? process.cwd(); + const candidates = [base, join(base, ".."), join(base, "..", "..")].slice(0, maxTries); + + const tried = []; + for (const c of candidates) { + tried.push(c); + const ok = verify(c); + if (ok) return { ...ok, tried }; + } + return { root: null, tried, reason: `none of ${tried.length} candidate(s) contained any of: ${repos.join(", ") || "(no repos given)"}` }; +} + +/** + * Resolve, once, which of `repos` are readable locally at their pinned shas. + * The result is meant to be persisted (evidence file) so that every later + * coordinator reads a map instead of re-probing the filesystem. + * + * `pins` is `{ "org/repo": "" }` — normally the deployState shas Step 4 + * already computed. + */ +export function resolveLocalRepos({ repos, pins = {}, workspaceRoot, branch, allowFetch = false }) { + const out = {}; + for (const repo of repos) { + const dir = localCloneFor(repo, workspaceRoot); + if (!dir) { out[repo] = { dir: null, usable: false, reason: "no local clone" }; continue; } + const sha = pins[repo]; + if (!sha) { out[repo] = { dir, usable: false, reason: "no pinned sha for this repo" }; continue; } + let present = hasCommit(dir, sha); + let fetched = false; + if (!present && allowFetch) { fetched = ensureCommit(dir, sha, branch); present = fetched; } + out[repo] = present + ? { dir, sha, usable: true, fetched } + : { dir, sha, usable: false, reason: `commit ${sha} not present locally${allowFetch ? " even after fetch" : ""}` }; + } + return out; +} + +/** Is this exact commit present locally? The only question that matters — + * a present commit is immutable, so its content cannot be stale. */ +export function hasCommit(dir, sha) { + try { + git(dir, ["cat-file", "-e", `${sha}^{commit}`]); + return true; + } catch { + return false; + } +} + +/** Make `sha` available locally with one targeted fetch. Returns true if the + * commit is present afterwards. Fetch touches only remote-tracking refs — it + * never moves a branch or the working tree, so it is safe to run against a + * repo someone is working in. */ +export function ensureCommit(dir, sha, branch) { + if (hasCommit(dir, sha)) return true; + try { + git(dir, ["fetch", "--quiet", "origin", branch ?? sha]); + } catch { + return false; + } + return hasCommit(dir, sha); +} + +/** + * Read one file at one commit. Returns + * `{ ok, content, source: "local"|"remote-needed", reason }`. + * + * Deliberately does NOT fall back to the network itself — it reports that the + * caller should. Keeping the decision at the edge means a wrong-looking local + * answer can never be silently substituted for the real one. + */ +export function readFileAt({ repo, sha, path, workspaceRoot, branch, allowFetch = false }) { + if (!SHA.test(String(sha ?? ""))) { + // Refusing a branch name is the point — see the header. + return { ok: false, source: "remote-needed", reason: `ref must be a commit sha, got ${JSON.stringify(sha)} (a branch name can silently read stale code)` }; + } + const dir = localCloneFor(repo, workspaceRoot); + if (!dir) return { ok: false, source: "remote-needed", reason: `no local clone of ${repo} under ${workspaceRoot}` }; + + if (!hasCommit(dir, sha)) { + if (!allowFetch) return { ok: false, source: "remote-needed", reason: `commit ${sha} not present in ${dir} (pass allowFetch to fetch it once)` }; + if (!ensureCommit(dir, sha, branch)) { + return { ok: false, source: "remote-needed", reason: `commit ${sha} still absent after fetch` }; + } + } + try { + return { ok: true, source: "local", content: git(dir, ["show", `${sha}:${path}`]), dir }; + } catch (err) { + // A missing path at that commit is a real answer, not a fallback trigger: + // the file genuinely did not exist there. + const msg = String(err.stderr ?? err.message ?? ""); + if (/does not exist|exists on disk, but not in/i.test(msg)) { + return { ok: false, source: "local", reason: `path not present at ${sha}: ${path}` }; + } + return { ok: false, source: "remote-needed", reason: msg.slice(0, 200) }; + } +} diff --git a/lib/signature.mjs b/lib/signature.mjs index 42dc0ae..0705835 100644 --- a/lib/signature.mjs +++ b/lib/signature.mjs @@ -76,3 +76,85 @@ export function clusterRows(rows) { return { rows, clusters }; } + +/** + * Seed-free, persist-safe clustering: read the CSV, assign `cluster_id`, and + * WRITE IT BACK. Returns the cluster objects. + * + * `clusterRows` mutates its input and returns `{rows, clusters}`, so a caller + * that destructures only `clusters` gets working cluster objects while every + * `cluster_id` is silently discarded — the CSV keeps empty cluster columns and + * the run degrades to one coordinator per test, losing the entire + * representative/sibling collapse. That is not a hypothetical: it happened on + * a real run (12 tests → 26 subagents, 30 minutes), and again to a second + * caller the same day. Two independent callers making the same mistake is an + * API problem, not a user problem. + * + * Prefer this over calling `clusterRows` directly whenever the rows came from + * a CSV. It cannot forget to persist. + */ +export function clusterAndPersist(csvPath, csvState) { + const { readRows, writeRows } = csvState; + const rows = readRows(csvPath); + const { clusters } = clusterRows(rows); + writeRows(csvPath, rows); + const persisted = readRows(csvPath).filter((r) => r.cluster_id).length; + if (rows.length && persisted !== rows.length) { + throw new Error( + `[signature] clustering wrote ${persisted}/${rows.length} cluster_id values — refusing to continue with a partially clustered CSV`, + ); + } + return clusters; +} + +/** + * Build the `pre_seed` a cluster sibling needs, from its representative's + * already-landed CSV row. Returns `{ok:false, reason}` if the representative + * is not terminal yet — meaning the sibling MUST NOT be dispatched. + * + * Siblings are only cheap because they confirm a hypothesis someone else + * already established. Dispatch one without that hypothesis and "one-turn + * confirm" degenerates into a full independent investigation — with the + * sibling framing on top, so it costs MORE than the representative it was + * meant to be a fraction of. Measured on a real run: siblings averaged 22.7 + * tool calls and 2.2 turns against 8.0 and 2.0 for the representative, and + * one burned 60 calls over 17 minutes. Nothing in the fan-out ordered them + * after their rep, and nothing refused to dispatch without a seed, so the + * degradation was silent. + * + * Fan-out contract: for each cluster, dispatch the representative, WAIT for it + * to land terminal, then dispatch its siblings with this seed. Clusters are + * independent, so they still run concurrently with each other. + */ +export function siblingPreSeed(csvPath, csvState, clusterId, representativeId) { + const rows = csvState.readRows(csvPath); + const rep = rows.find((r) => String(r.testRunId) === String(representativeId)); + if (!rep) return { ok: false, reason: `representative ${representativeId} not in the CSV` }; + + const state = String(rep.rca_done ?? "").toLowerCase(); + if (state !== "resolved") { + return { + ok: false, + reason: `representative ${representativeId} is "${rep.rca_done || "pending"}", not resolved — dispatching siblings now would make each one re-investigate from scratch`, + }; + } + if (!String(rep.root_cause ?? "").trim()) { + return { ok: false, reason: `representative ${representativeId} resolved but recorded no root_cause — nothing for a sibling to confirm` }; + } + + return { + ok: true, + clusterId, + representativeId: String(representativeId), + pre_seed: { + cause: rep.root_cause, + failure_type: rep.failure_type || "", + related_prs: rep.related_prs || "", + confidence: rep.confidence || "", + // Stated so the sibling confirms against ITS OWN evidence rather than + // adopting the verdict — the independence rule in Operating Principle 0. + instruction: + "Confirm or refute this against YOUR OWN test's evidence in one turn. Do not adopt it because it is written here.", + }, + }; +} diff --git a/lib/state-dir.mjs b/lib/state-dir.mjs new file mode 100644 index 0000000..d91570e --- /dev/null +++ b/lib/state-dir.mjs @@ -0,0 +1,136 @@ +// Housekeeping for the shared state directory (`/bstack-rca/`). +// +// Everything a run produces — the state CSV, the evidence file and its +// contribution shards, the tool cache — lands here and is NEVER deleted by the +// run itself. That is deliberate: resume is keyed on buildId → same path, so +// cleaning up on completion would break `pending-resume`. The cost is that the +// directory accumulates, and that artifacts written by older versions keep +// whatever permissions they were created with. +// +// Both problems need a sweep rather than a per-write fix, because a write only +// ever touches the one file it is writing. `hardenStateDir` is cheap enough to +// run unconditionally at gate startup; `pruneStateDir` is deliberately NOT +// automatic (see below). + +import { existsSync, readdirSync, statSync, chmodSync, rmSync } from "node:fs"; +import { join } from "node:path"; + +/** + * Make the whole state tree owner-only, repairing anything left open by an + * older version. + * + * Per-write hardening can't do this: `writeRows` tightens the file it writes + * and nothing else, so a build analysed before the hardening landed keeps its + * 0644 forever unless something rewrites it — and a completed build never gets + * rewritten. Measured on a real machine: the directory itself was drwxr-xr-x + * and 6 files were still 0644, holding root causes, culprit PRs and log + * excerpts in a shared OS temp dir. + * + * Never throws: a file owned by another user is skipped, because failing the + * whole RCA run over one un-chmod-able leftover would be a worse outcome than + * the leak we're closing. + * + * Returns `{ dirs, files, skipped }` counts. + */ +export function hardenStateDir(dir) { + const out = { dirs: 0, files: 0, skipped: [] }; + if (!dir || !existsSync(dir)) return out; + + const walk = (p) => { + let st; + try { + st = statSync(p); + } catch { + out.skipped.push(p); + return; + } + const isDir = st.isDirectory(); + const want = isDir ? 0o700 : 0o600; + if ((st.mode & 0o777) !== want) { + try { + chmodSync(p, want); + } catch { + out.skipped.push(p); + return; // can't chmod it; don't pretend we descended into it either + } + } + if (isDir) { + out.dirs++; + let entries = []; + try { + entries = readdirSync(p); + } catch { + out.skipped.push(p); + return; + } + for (const e of entries) walk(join(p, e)); + } else { + out.files++; + } + }; + + walk(dir); + return out; +} + +/** + * Delete build artifacts older than `maxAgeMs` (default 7 days). + * + * NOT called automatically, and the default is deliberately far longer than + * any run: these files ARE the resume state, so anything that deletes them can + * silently turn a resumable build into a lost one. Seven days is well past the + * minutes a batch takes while still bounding growth, and the caller has to ask + * for it explicitly. + * + * `dryRun: true` reports what would go without touching anything — use it + * before wiring this into anything automatic. + * + * Returns `{ removed, bytes, kept, dryRun }`. + */ +export function pruneStateDir(dir, nowMs, { maxAgeMs = 7 * 24 * 60 * 60 * 1000, dryRun = false } = {}) { + const res = { removed: [], bytes: 0, kept: 0, dryRun }; + if (!dir || !existsSync(dir)) return res; + + const sizeOf = (p) => { + let total = 0; + const st = statSync(p); + if (!st.isDirectory()) return st.size; + for (const e of readdirSync(p)) { + try { + total += sizeOf(join(p, e)); + } catch { /* vanished mid-walk */ } + } + return total; + }; + + for (const name of readdirSync(dir)) { + const p = join(dir, name); + let st; + try { + st = statSync(p); + } catch { + continue; + } + // mtime, not atime: reading an evidence file during a resume should not + // make a long-abandoned build look freshly relevant. + if (nowMs - st.mtimeMs <= maxAgeMs) { + res.kept++; + continue; + } + let bytes = 0; + try { + bytes = sizeOf(p); + } catch { /* best effort */ } + if (!dryRun) { + try { + rmSync(p, { recursive: true, force: true }); + } catch { + res.kept++; + continue; + } + } + res.removed.push(name); + res.bytes += bytes; + } + return res; +} diff --git a/lib/theme-clustering.mjs b/lib/theme-clustering.mjs new file mode 100644 index 0000000..3826ef8 --- /dev/null +++ b/lib/theme-clustering.mjs @@ -0,0 +1,71 @@ +// Server-computed failure-theme clustering (o11y `buildThemes`/`flat` via the +// getBuildFailureThemes/listTestsInFailureTheme MCP tools). Preferred over +// lib/signature.mjs's client-side text-signature clustering whenever the +// server-side computation is ready (skills/rca-build/SKILL.md Step 3). Falls +// back to lib/signature.mjs when getBuildFailureThemes reports `ready: false`. +// +// Pure + dependency-free, mirroring lib/signature.mjs's shape and +// testability: takes already-fetched plain data in, returns { rows, clusters } +// with the same shape clusterRows() produces, so downstream code (the fan-out +// workflow, the sequential harness) doesn't care which path produced it. + +import { selectRepresentative } from "./signature.mjs"; + +// Build { rows, clusters } from a getBuildFailureThemes result (`ready: true`) +// plus a per-theme map of already-fetched member rows (keyed by +// buildFailureThemeId, each entry the array listTestsInFailureTheme returned +// for that theme, already paginated to completion). `rows` is the full +// listTestIds row set — used to enrich each theme member with the row's own +// testName/error_summary and to catch any failed test the server didn't +// assign to a theme: never silently dropped, it becomes its own singleton, +// same convention as lib/signature.mjs. Mutates each row's `cluster_id`, same +// as `clusterRows()`. +// +// Themes are expected to be disjoint (a test belongs to at most one), but +// this isn't a guarantee the server's contract documents — so a row already +// claimed by an earlier theme is skipped (first-theme-wins) rather than +// letting it land in two clusters with conflicting `cluster_id` values. +export function clustersFromThemes(rows, themesResult, testsByThemeId) { + const rowById = new Map(rows.map((r) => [String(r.testRunId), r])); + const covered = new Set(); + const clusters = []; + + for (const theme of themesResult.buildThemes ?? []) { + const themeRows = (testsByThemeId[theme.buildFailureThemeId] ?? []) + .map((t) => rowById.get(String(t.testRunId))) + .filter((r) => r && !covered.has(String(r.testRunId))); + + if (themeRows.length === 0) continue; + + const id = `theme-${theme.buildFailureThemeId}`; + themeRows.forEach((r) => { + r.cluster_id = id; + covered.add(String(r.testRunId)); + }); + + const representative = selectRepresentative(themeRows); + const siblings = themeRows.filter((m) => m !== representative); + clusters.push({ + cluster_id: id, + signature: theme.themeData?.name ?? "", + members: themeRows, + representative, + siblings, + }); + } + + for (const row of rows) { + if (covered.has(String(row.testRunId))) continue; + const id = `solo-${row.testRunId}`; + row.cluster_id = id; + clusters.push({ + cluster_id: id, + signature: "", + members: [row], + representative: row, + siblings: [], + }); + } + + return { rows, clusters }; +} diff --git a/lib/tool-cache.mjs b/lib/tool-cache.mjs new file mode 100644 index 0000000..268e78f --- /dev/null +++ b/lib/tool-cache.mjs @@ -0,0 +1,401 @@ +// Build-scoped memo cache for READ-ONLY tool calls (`gh`, `kubectl`, curl, …). +// +// The evidence file (`evidence-file.mjs`) shares *digested findings* whose +// shape the schema knows about — deploy state, PRs, log sweeps. But a +// coordinator's real tool traffic is mostly raw lookups that fit no schema +// slot: fetching a spec file's contents at a ref, listing a git tree, running +// a code search. Measured on one real 10-test build: `gh` was 37% of all +// coordinator tool calls, and 46 of them were byte-identical commands re-run +// by different coordinators — the same BStackAutomation spec fetched 12 +// times, a frontend component 5 times. +// +// This module memoizes at the CALL level instead, so duplication is caught +// regardless of what the call was for. Any coordinator about to run a +// read-only command checks here first; on a miss it runs the command and +// stores the result for everyone else. +// +// CONCURRENCY: one file per cache KEY (`.json`), not one per writer. +// Distinct calls write distinct files; two agents racing on the *same* call +// write byte-identical content, so the race is benign. Writes go through a +// temp file + `rename`, which is atomic on POSIX, so a reader never observes +// a half-written entry. No locking, no lost updates, no torn reads. +// +// WHAT IS NOT CACHED (deliberate): +// - Any command that fails (non-zero exit). A transient `gh` rate-limit or +// an expired kube token must never be memoized into a persistent "answer" +// that poisons every later reader. +// - Anything matching the mutation denylist below. The plugin is read-only +// by contract, but caching is a correctness-sensitive place to trust that +// contract blindly, so mutations are refused defensively. +// - Secrets: values that look like tokens/passwords are redacted from the +// stored payload before it ever touches disk. + +import { + readFileSync, writeFileSync, existsSync, mkdirSync, readdirSync, renameSync, chmodSync, +} from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { createHash } from "node:crypto"; + +/** Per-build cache directory, sitting alongside the state CSV and evidence + * file under the same OS-temp convention. */ +export function toolCacheDirFor(buildId, stateDir = "") { + const safe = String(buildId ?? "").replace(/[^A-Za-z0-9._-]/g, "_") || "unknown-build"; + const dir = stateDir && String(stateDir).trim() !== "" ? String(stateDir) : join(tmpdir(), "bstack-rca"); + return join(dir, `rca-toolcache.${safe}`); +} + +/** Stable key for one call. Whitespace is normalized so trivially-different + * formatting of the same command still hits, but nothing else is rewritten — + * `| head -20` vs `| head -200` genuinely return different output and must + * stay distinct keys. */ +// Owner-only, on create AND on an existing directory. `mkdirSync`'s `mode` +// applies only when it creates the dir, so one made before this hardening +// landed keeps its 0755 forever — and these artifacts hold root causes, +// culprit PRs and log excerpts in a shared OS temp dir. Found in practice: +// /bstack-rca was drwxr-xr-x with 0600 files inside it. +function ensureOwnerOnlyDir(dir) { + if (!existsSync(dir)) { mkdirSync(dir, { recursive: true, mode: 0o700 }); return; } + try { chmodSync(dir, 0o700); } catch { /* not ours to tighten; leave it */ } +} + +export function cacheKey(command) { + const norm = String(command ?? "").replace(/\s+/g, " ").trim(); + return createHash("sha256").update(norm).digest("hex").slice(0, 24); +} + +// Commands that must never be memoized, even if someone wires this into a +// non-read-only context by mistake. +// Note: output redirection is deliberately NOT in this list. A `>/dev/null` +// is a redirect, not a mutation, and lumping the two together produced the +// misleading refusal "command looks mutating" for ordinary read-only calls. +// Redirects are handled separately, with an accurate message. +const MUTATING = /\b(rm|mv|cp|dd|truncate|tee)\b|\bgit\s+(push|commit|merge|rebase|reset|checkout|clean)\b|\bgh\s+(pr\s+(create|merge|close|edit|comment|review)|issue\s+(create|close|edit|comment)|release\s+create|repo\s+(create|delete)|api\s+(-X\s*)?(POST|PUT|PATCH|DELETE))|\bkubectl\s+(apply|delete|edit|patch|scale|create|replace|annotate|label|cordon|drain|exec|cp|port-forward|rollout\s+(undo|restart|pause|resume))\b|\bcurl\b[^|]*\s-(X|-request)\s*(POST|PUT|PATCH|DELETE)/i; + +/** + * Strip stderr-plumbing that the wrapper already handles. + * + * `2>&1` and `2>/dev/null` appear on the majority of real recorded calls — + * agents add them reflexively because `gh` is chatty. They say nothing about + * WHAT to fetch, only where stderr should go, and the wrapper captures stderr + * separately regardless. Refusing them rejected 134 of 223 recorded calls and + * drove the effective hit rate to zero, so they are normalized away instead. + * + * Genuine FILE redirects (`> out.json`, `>> log`, `< in`) are left in place so + * the check below still refuses them: those change where data goes, which the + * wrapper cannot honour while also returning stdout to the caller. + */ +function stripStderrPlumbing(seg) { + return String(seg ?? "") + .replace(/\s*2>&1\s*/g, " ") + .replace(/\s*2>\s*\/dev\/null\s*/g, " ") + .trim(); +} + +/** True if the segment still contains a real redirect outside quotes, after + * stderr plumbing has been normalized away. */ +function hasUnquotedRedirect(seg) { + let quote = null; + for (const ch of String(seg ?? "")) { + if (quote) { if (ch === quote) quote = null; continue; } + if (ch === "'" || ch === '"') { quote = ch; continue; } + if (ch === ">" || ch === "<") return true; + } + return false; +} + +export function isCacheable(command) { + return !MUTATING.test(String(command ?? "")); +} + +// Read-only data-fetching binaries this wrapper will run. Anything else is +// refused outright. +const ALLOWED_LEADER = /^\s*(gh|kubectl|curl|git)\s/; + +// Pure text filters allowed AFTER the fetch in a pipeline. They transform the +// fetch's output and never reach the network, so they are deliberately outside +// the cache key: `gh api X | jq .a` and `gh api X | jq .b` share ONE cached +// fetch. Measured motivation — replaying real recorded traffic, 89% of calls +// embedded the fetch in a pipeline, so refusing pipelines outright meant the +// cache applied to almost nothing in practice. +const ALLOWED_FILTER = new Set([ + "jq", "grep", "egrep", "head", "tail", "sort", "uniq", "wc", + "cut", "tr", "sed", "awk", "base64", "python3", "rev", "column", +]); + +/** Split on top-level `|` only — a pipe inside quotes (a jq expression) stays + * part of its segment. Returns trimmed segment strings. */ +export function splitPipeline(command) { + const segs = []; + let cur = ""; + let quote = null; + const s = String(command ?? ""); + for (let i = 0; i < s.length; i++) { + const ch = s[i]; + // A backslash escapes the next character. Without this, `\"` inside a + // double-quoted jq filter reads as "close quote", the parser thinks it is + // back outside quotes, and a `|` in a regex alternation like + // `test("vite|env";"i")` gets split as a shell pipe. + if (ch === "\\" && i + 1 < s.length && quote !== "'") { + cur += ch + s[i + 1]; + i++; + continue; + } + if (quote) { + cur += ch; + if (ch === quote) quote = null; + continue; + } + if (ch === "'" || ch === '"') { quote = ch; cur += ch; continue; } + if (ch === "|") { segs.push(cur.trim()); cur = ""; continue; } + cur += ch; + } + segs.push(cur.trim()); + return segs.filter((s2) => s2.length > 0); +} + +// Shell operators, checked as whole ARGV TOKENS rather than by scanning the +// raw string. Scanning the string was too blunt and refused legitimate reads: +// `--jq 'test("rcaThree";"i")'` was rejected for the `;` inside a quoted jq +// expression, and `search/code?q=X&per_page=20` for the `&` inside a URL. +// +// Post-tokenization this distinction is exact: quoted metacharacters end up +// *inside* an argument (harmless — we execFile, so no shell ever interprets +// them), while a real operator survives as its own standalone token. +const OPERATOR_TOKENS = new Set([";", "|", "||", "&&", "&", ">", ">>", "<", "<<"]); + +/** + * Gate for `bin/cached-exec.mjs`. Returns `{ ok, reason }`. + * + * Scope note, stated plainly: this is defense-in-depth, NOT a security + * boundary. The only caller is a coordinator agent that already has direct + * shell access via its Bash tool, so the wrapper grants no capability the + * caller lacks and cannot meaningfully contain a caller that wants to misuse + * it. What it does buy: a fat-fingered or model-hallucinated command can't + * quietly run something mutating *through the cache path* and get memoized, + * and refusing `;`-chained loops nudges callers toward one-fetch-per-call, + * which caches far better anyway. + */ +/** + * Validate a command and return its execution plan. + * + * On success: `{ ok, fetch: string[], filters: string[][] }` — the fetch is + * executed (or served from cache) and its output is piped through the filters, + * each run via execFile with NO shell anywhere in the chain. + * + * Pipelines are accepted rather than refused because refusing them is what + * made the cache useless on real traffic. Only the FETCH is keyed, so several + * agents filtering one fetch differently all share a single cached result. + */ +export function isRunnable(command) { + const c = String(command ?? ""); + if (!isCacheable(c)) return { ok: false, reason: "command looks mutating" }; + + const segments = splitPipeline(c).map(stripStderrPlumbing).filter((s) => s.length > 0); + if (segments.length === 0) return { ok: false, reason: "empty command" }; + + if (!ALLOWED_LEADER.test(segments[0])) { + return { ok: false, reason: "command must start with gh, kubectl, curl, or git" }; + } + + const parsed = []; + for (const seg of segments) { + if (hasUnquotedRedirect(seg)) { + return { + ok: false, + reason: "file redirects (>, >>, <) are not supported — the wrapper returns stdout to you directly, so drop the redirect. (`2>&1` and `2>/dev/null` are fine; they're stripped automatically.)", + }; + } + let argv; + try { + argv = tokenize(seg); + } catch (err) { + return { ok: false, reason: err.message }; + } + if (argv.length === 0) return { ok: false, reason: "empty pipeline segment" }; + const op = argv.find((t) => OPERATOR_TOKENS.has(t)); + if (op) { + return { + ok: false, + reason: `'${op}' is a shell operator. Pipes are supported, but ';', '&&', redirects and substitution are not — issue one fetch per call.`, + }; + } + parsed.push(argv); + } + + for (const argv of parsed.slice(1)) { + if (!ALLOWED_FILTER.has(argv[0])) { + return { + ok: false, + reason: `'${argv[0]}' is not an allowed filter after the fetch (allowed: ${[...ALLOWED_FILTER].join(", ")})`, + }; + } + } + + return { ok: true, fetch: parsed[0], filters: parsed.slice(1), fetchText: segments[0] }; +} + +/** + * Split a command string into argv the way a shell would for the simple cases + * we allow — honouring single/double quotes — WITHOUT invoking a shell. The + * caller then runs `execFile(argv[0], argv.slice(1))`, so no shell ever + * interprets metacharacters and command injection has no surface. Throws on + * an unterminated quote rather than guessing. + */ +export function tokenize(command) { + const out = []; + let cur = ""; + let quote = null; + let started = false; + const s = String(command ?? ""); + for (let i = 0; i < s.length; i++) { + const ch = s[i]; + // Backslash escape, POSIX-style: literal everywhere except inside single + // quotes. Missing this mangled jq's most common idiom — `\"` was treated + // as a quote delimiter, so `select(.filename==\"x\")` reached the binary + // as `select(.filename==\x\)` with the quotes eaten. + if (ch === "\\" && i + 1 < s.length && quote !== "'") { + cur += s[i + 1]; + i++; + started = true; + continue; + } + if (quote) { + if (ch === quote) quote = null; + else cur += ch; + continue; + } + if (ch === "'" || ch === '"') { quote = ch; started = true; continue; } + if (/\s/.test(ch)) { + if (started) { out.push(cur); cur = ""; started = false; } + continue; + } + cur += ch; + started = true; + } + if (quote) throw new Error("unterminated quote in command"); + if (started) out.push(cur); + return out; +} + +// ---- MCP calls ------------------------------------------------------------ + +// Stateful MCP tools that must NEVER be memoized. `tfaRcaTurn` advances a +// conversation and `getTfaTurnResult` reads a turn whose status is *expected* +// to change between reads — serving either from cache would be actively +// wrong, not merely stale. +const MCP_NEVER = /tfaRcaTurn|getTfaTurnResult|triggerRcaReport/i; + +export function isCacheableMcp(toolName) { + return !MCP_NEVER.test(String(toolName ?? "")); +} + +/** Key an MCP call by tool name + canonicalized args (object keys sorted), so + * the same query written with its arguments in a different order still hits. */ +export function mcpCacheKey(toolName, args) { + const canon = (v) => { + if (Array.isArray(v)) return v.map(canon); + if (v && typeof v === "object") { + return Object.keys(v).sort().reduce((a, k) => { a[k] = canon(v[k]); return a; }, {}); + } + return v; + }; + const payload = JSON.stringify({ tool: String(toolName ?? ""), args: canon(args ?? {}) }); + return createHash("sha256").update(payload).digest("hex").slice(0, 24); +} + +// Redact the secret VALUE, bounded by the first structural delimiter — never +// "the rest of the line". +// +// The rest-of-line version silently destroyed data. GitHub's file API returns +// SINGLE-LINE JSON whose `download_url` always carries `?token=…`, so matching +// `token=` and consuming `[^\r\n]*` swallowed the entire remaining payload: +// a 214KB response cached as 816 bytes, content field gone, no warning. Every +// private-repo file fetch was affected. +// +// A value therefore stops at whitespace, quote, comma, semicolon, brace, +// bracket or `&` — enough to cover a real credential, never enough to eat the +// surrounding document. +const SECRET_KV = + /((?:token|authorization|api[_-]?key|secret|password|passwd|access[_-]?key)"?\s*[=:]\s*"?)((?:bearer|basic|token)\s+)?([^\s"'`,;}\]&\r\n]{4,})/gi; + +// A bare `Bearer ` / `Basic ` with no key= in front of it. +const SECRET_SCHEME = /\b(bearer|basic)\s+([A-Za-z0-9._~+/=-]{8,})/gi; + +/** Redact anything token-shaped before it is persisted. The cache lives in + * temp, but a cached `gh api` response or log line could still carry a + * credential, and "it's only temp" is not a reason to write one to disk. */ +export function redact(text) { + return String(text ?? "") + .replace(SECRET_KV, (_m, key) => `${key}`) + .replace(SECRET_SCHEME, (_m, scheme) => `${scheme} `); +} + +const MAX_BYTES = 256 * 1024; + +let tmpSeq = 0; + +export function cacheGet(cacheDir, key) { + const p = join(cacheDir, `${key}.json`); + if (!existsSync(p)) return null; + try { + return JSON.parse(readFileSync(p, "utf8")); + } catch { + return null; // half-written or corrupt -> treat as a miss, never throw + } +} + +/** Atomic write: temp file + rename, so concurrent readers only ever see a + * complete entry. `nowMs` is passed in (same clock discipline as the rest of + * lib/). Returns the stored entry. */ +export function cachePut(cacheDir, key, entry, nowMs) { + // Owner-only (0700 dir / 0600 files). The cache lives under a world-readable + // OS temp dir and holds raw `gh`/`kubectl` output — private repo source, + // internal hostnames, log bodies. `redact()` below is best-effort pattern + // matching and will not catch everything, so the filesystem permission is + // the actual control, not a backstop. + // + // The FILE mode is the load-bearing part: a pre-existing directory (e.g. a + // `stateDir` the user already created, or one left by an earlier run) keeps + // its own permissions, since silently chmod'ing a path we were handed would + // be presumptuous. Entries stay 0600 regardless, and a traversable directory + // only exposes opaque hash filenames, not their contents. + ensureOwnerOnlyDir(cacheDir); + const raw = redact(entry.stdout ?? ""); + const truncated = raw.length > MAX_BYTES; + const rec = { + key, + command: entry.command, + writerId: entry.writerId ?? null, + capturedAtMs: nowMs, + exitCode: entry.exitCode ?? 0, + truncated, + bytes: raw.length, + stdout: truncated ? raw.slice(0, MAX_BYTES) + "\n… [truncated by tool-cache]" : raw, + }; + const finalPath = join(cacheDir, `${key}.json`); + // pid + counter keeps the temp name unique per writer, so `mode` genuinely + // applies (it is honoured on create, not on truncate of an existing file). + const tmpPath = join(cacheDir, `.${key}.${process.pid}.${tmpSeq++}.tmp`); + writeFileSync(tmpPath, JSON.stringify(rec, null, 2), { encoding: "utf8", mode: 0o600 }); + renameSync(tmpPath, finalPath); // atomic on POSIX; preserves the 0600 mode + return rec; +} + +/** Cache-wide counters for the run's summary — how much duplicate work this + * actually saved, rather than assuming it saved any. */ +export function cacheStats(cacheDir) { + if (!existsSync(cacheDir)) return { entries: 0, bytes: 0 }; + let entries = 0; + let bytes = 0; + for (const f of readdirSync(cacheDir)) { + if (!f.endsWith(".json") || f.startsWith(".")) continue; + entries++; + try { + bytes += JSON.parse(readFileSync(join(cacheDir, f), "utf8")).bytes ?? 0; + } catch { + /* skip */ + } + } + return { entries, bytes }; +} diff --git a/lib/turn1-registry.mjs b/lib/turn1-registry.mjs new file mode 100644 index 0000000..d0674c9 --- /dev/null +++ b/lib/turn1-registry.mjs @@ -0,0 +1,117 @@ +// Step 4b pre-dispatch registry (see skills/rca-build/SKILL.md Step 4b). +// +// Step 4b submits tfaRcaTurn's FIRST turn for every cluster representative +// directly from the orchestrator, in the same tool-call batch as Step 4's +// evidence pre-fetch — so the representative's turn 1 is already in flight +// (or already answered) by the time Step 5 would otherwise submit it fresh. +// +// A RESOLVED turn 1 needs no registry entry at all: the orchestrator flips +// that row straight to terminal in the CSV (lib/csv-state.mjs) and Step 5 +// skips dispatching a coordinator for it entirely. Only the two non-terminal +// outcomes are recorded here, for Step 5 to hand to the representative's +// coordinator instead of letting it submit turn 1 again: +// - PENDING -> {threadId, turnId} (drain via the existing `resume` input) +// - NEEDS_INFO -> {threadId, asks} (new `turn1_result` input — see +// agents/ai-tfa-coordinator.md and lib/loop.mjs) +// +// Single-writer: only the Step 4b orchestrator pass ever writes this file (one +// process, one point in time, before any coordinator is dispatched). Step 5 +// only reads it once per representative while building dispatch prompts, so — +// unlike the evidence file's per-coordinator shards — a plain read-modify-write +// is safe; there is no concurrent-writer race to design around here. +// +// Path convention mirrors csvPathFor/evidencePathFor exactly: build id in the +// filename, OS temp by default, `stateDir` overrides the directory only. + +import { readFileSync, writeFileSync, existsSync, mkdirSync, chmodSync, rmSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { tmpdir } from "node:os"; + +const safeName = (v, fallback) => + String(v ?? "").replace(/[^A-Za-z0-9._-]/g, "_") || fallback; + +export function turn1PathFor(buildId, stateDir = "") { + const safe = safeName(buildId, "unknown-build"); + const dir = stateDir && String(stateDir).trim() !== "" ? String(stateDir) : join(tmpdir(), "bstack-rca"); + return join(dir, `rca-turn1.${safe}.json`); +} + +// Owner-only, on create AND on an existing directory — same rationale as +// csv-state.mjs / evidence-file.mjs: a directory made before this hardening +// landed keeps its 0755 forever, and this file carries thread ids and NEEDS_INFO +// ask text in a shared OS temp dir. +function ensureOwnerOnlyDir(dir) { + if (!existsSync(dir)) { mkdirSync(dir, { recursive: true, mode: 0o700 }); return; } + try { chmodSync(dir, 0o700); } catch { /* not ours to tighten; leave it */ } +} + +function emptyRegistry(buildId, nowMs) { + return { buildId: String(buildId ?? ""), generatedAtMs: nowMs, entries: {} }; +} + +function readDoc(filePath) { + if (!existsSync(filePath)) return null; + try { + return JSON.parse(readFileSync(filePath, "utf8")); + } catch { + return null; + } +} + +function writeDoc(filePath, doc) { + const dir = dirname(filePath); + if (dir) ensureOwnerOnlyDir(dir); + const existed = existsSync(filePath); + writeFileSync(filePath, JSON.stringify(doc, null, 2), { encoding: "utf8", mode: 0o600 }); + // `mode` is only honoured on create — tighten a pre-hardening leftover too. + if (existed) chmodSync(filePath, 0o600); +} + +/** Idempotent: creates the file with the given `buildId` if it doesn't exist + * yet; leaves an existing file untouched otherwise (never clobbers prior + * entries on a resume). */ +export function initTurn1Registry(filePath, buildId, nowMs) { + const existing = readDoc(filePath); + if (existing) return existing; + const doc = emptyRegistry(buildId, nowMs); + writeDoc(filePath, doc); + return doc; +} + +/** + * Record a representative's non-terminal turn-1 outcome. + * `entry` shape: `{ threadId, turnId, status: "PENDING" | "NEEDS_INFO", asks, note }`. + * `turnId` only applies to PENDING (per the tfaRcaTurn contract — RESOLVED and + * NEEDS_INFO never carry one). `asks` only applies to NEEDS_INFO. + * Read-modify-write against the whole file — safe because Step 4b is this + * file's only writer. + */ +export function recordTurn1(filePath, testRunId, entry, nowMs) { + const doc = readDoc(filePath) ?? emptyRegistry("unknown-build", nowMs); + doc.entries[String(testRunId)] = { ...entry, submittedAtMs: nowMs }; + doc.generatedAtMs = nowMs; + writeDoc(filePath, doc); + return doc.entries[String(testRunId)]; +} + +/** This representative's pre-dispatched turn-1 outcome, or `null` if Step 4b + * never ran for it (not clustered as a representative, resolved already and + * flipped straight to the CSV, or the registry doesn't exist at all). */ +export function readTurn1(filePath, testRunId) { + const doc = readDoc(filePath); + return doc?.entries?.[String(testRunId)] ?? null; +} + +/** Every recorded entry, keyed by testRunId — used for run-end stats only. */ +export function readAllTurn1(filePath) { + return readDoc(filePath)?.entries ?? {}; +} + +/** Used by lib/build-cleanup.mjs once the build's report has generated + * successfully. A missing file is not an error — Step 4b may never have run + * (no clusters, or every representative resolved on turn 1). */ +export function deleteTurn1Registry(filePath) { + if (!existsSync(filePath)) return false; + rmSync(filePath, { force: true }); + return true; +} diff --git a/skills/rca-build/SKILL.md b/skills/rca-build/SKILL.md index e979813..2c4be5e 100644 --- a/skills/rca-build/SKILL.md +++ b/skills/rca-build/SKILL.md @@ -11,19 +11,133 @@ logs; the client agent owns everything else** (product code, infra/runtime, logs metrics, deploy, ci) — routed by capability, generic over product and infra. This skill is the **build-level orchestrator** (`ai-tfa-orchestrator` role). It -never calls `tfaRcaTurn` itself — it dispatches the `ai-tfa-coordinator` -(test-level) per test/cluster member, which drives the loop and lets TFA author -the dashboard RCA. **The full RCA report lives on the Test Observability UI, not -in Claude** — this run's job is to feed it, then surface a terse glimpse and the +dispatches the `ai-tfa-coordinator` (test-level) per test/cluster member, which +drives the loop and lets TFA author the dashboard RCA — the one narrow +exception is Step 4b's turn-1 pre-dispatch, a single direct `tfaRcaTurn` call +per cluster representative, concurrent with Step 4. **The full RCA report +lives on the Test Observability UI, not in Claude** — this run's job is to feed it, then surface a terse glimpse and the link. There is exactly **one mode**: autonomous. There is exactly **one gate** (Step -1) before execution. After the gate closes, **the run never asks the user -anything again.** + +1. before execution. After the gate closes, **the run never asks the user + anything again.** Config (concurrency, turn-cap, paths, evidence registry) lives in `config/rca.config.json`. State lives in the CSV/WAL spine (`lib/csv-state.mjs`). + +For maximum efficiency, whenever you need to perform multiple independent +operations — connector probes, per-repo evidence fetches, per-workload log +sweeps, or any other set of calls with no dependency between them — invoke all +relevant tools simultaneously in one message rather than sequentially. +Prioritize calling tools in parallel whenever possible; err on the side of +maximizing parallel tool calls rather than running too many tools +sequentially. This applies throughout every step below (Gate probes, Step 4's +per-repo/per-workload pre-fetch, Step 4b's cluster dispatch, Step 5's +representative and sibling dispatch) — a real run measured this exact +violation costing 4+ minutes on gate probes alone. The only exception is when +one call's output is a literal input to another; that pair, and only that +pair, runs in order. + + +## API reference — read THIS, do not grep the source + +Every signature this run needs, in one place. This exists because agents were +routinely re-deriving these signatures live — `grep -n "^export function" +lib/…`, `cat config/…`, repeated `ls .claude/skills/` — a real, recurring tax +that grew every time a helper was added faster than the docs described it, so +the plugin taxed every agent to relearn itself from source instead of reading +one page. + +Everything below is product-neutral: build ids, repos, branches, workloads and +paths are all **inputs**, supplied by the gate and the connector skills. + +**State spine — `lib/csv-state.mjs`** +``` +csvPathFor(buildId, stateDir="") → /bstack-rca/rca-state..csv +seed(csvPath, buildId, tests) → rows; idempotent, preserves terminal rows +readRows(csvPath) / writeRows(csvPath,rows) throws on a foreign header rather than dropping columns +claim(csvPath, testRunId, worker, nowMs) → false if already claimed +heartbeat(csvPath, testRunId, worker, nowMs) +flip(csvPath, testRunId, fields, nowMs) → false if rca_done missing/non-terminal +reaper(csvPath, ttlSec, nowMs) → reclaimed ids +pendingRows(csvPath) → pending + pending-resume +``` + +**Clustering — `lib/signature.mjs`** +``` +clusterAndPersist(csvPath, csvStateModule) → clusters; WRITES cluster_id back. Use this. +siblingPreSeed(csvPath, csvState, clusterId, repId) → {ok, pre_seed} | {ok:false, reason} +clusterRows(rows) → {rows, clusters}; mutates, does NOT persist +``` + +**Shared evidence — `lib/evidence-file.mjs`** +``` +evidencePathFor(buildId, stateDir="") initEvidenceFile(path, buildId, nowMs) +setGithubEvidence(path, repo, entry, nowMs) setLogsEvidence(path, workload, entry, nowMs) +setBaseline(path, baseline, suspectWindow, nowMs) setLocalRepos(path, localRepos, nowMs) +contributeGithubEvidence(path, writerId, repo, patch, nowMs) ← coordinators write HERE +contributeLogsEvidence(path, writerId, workload, patch, nowMs) +deployShas(pathOrDoc) → {pins:{repo:sha}, source} recomputeCoverage(path, {repos,workloads}, nowMs) +readEvidenceFile(path) folds base+shards · readBaseFile(path) is base ONLY +``` + +**Local repo reads — `lib/repo-source.mjs`** +``` +discoverWorkspaceRoot({repos, from, explicit, maxTries=3}) → {root, matched, tried, reason} +resolveLocalRepos({repos, pins, workspaceRoot}) → {repo:{usable, sha|reason}} +readFileAt({repo, sha, path, workspaceRoot}) → sha ONLY; a branch name is refused +``` + +**Housekeeping — `lib/state-dir.mjs`** +``` +hardenStateDir(dir) run once at gate start; idempotent +pruneStateDir(dir, nowMs, {maxAgeMs, dryRun}) NOT automatic — these files are the resume state +``` + +**Step 4b turn-1 pre-dispatch registry — `lib/turn1-registry.mjs`** +``` +turn1PathFor(buildId, stateDir="") → /bstack-rca/rca-turn1..json +initTurn1Registry(path, buildId, nowMs) idempotent, never clobbers existing entries +recordTurn1(path, testRunId, {status, threadId, turnId?, asks?}, nowMs) PENDING or NEEDS_INFO only — RESOLVED is flipped straight into the CSV instead +readTurn1(path, testRunId) → entry | null +readAllTurn1(path) → {testRunId: entry} run-end stats only +deleteTurn1Registry(path) → boolean (existed?) called by lib/build-cleanup.mjs +``` + +**Build-completion cleanup — `lib/build-cleanup.mjs`** +``` +cleanupBuildArtifacts(buildId, stateDir="") → {deleted, errors} + deletes THIS build's CSV, evidence file + .contrib shards, tool cache dir, and turn1 registry. + Call ONLY after triggerRcaReport succeeds (Step 6) — never a periodic sweep, see lib/state-dir.mjs. +``` + +**Routing / output — `lib/routing.mjs`, `lib/glimpse.mjs`, `lib/evidence-cache.mjs`** +``` +loadConfig(configPath) buildManifest(config, discovered) routeAsks(asks, config, manifest) +renderGlimpseFromCsv(csvPath, {buildId}) resolveBaseline(lastGreenRef, fallbackRef) +``` + +**Commands — `bin/`** +``` +node bin/evidence-show.mjs [--summary | --prs | --repo ] +node bin/repo-read.mjs [--fetch] +node bin/cached-exec.mjs '' (pipe OUTSIDE the wrapper) +node bin/cached-mcp.mjs get|put '' +``` + +**Constants worth knowing** +``` +csv-state.COLUMNS the canonical column set; writeRows emits exactly these +csv-state.RESUMABLE "pending-resume" — a SOFT terminal: claim released, row still picked up +routing.TEST_LOGS the ask type TFA owns; never gather it, always skip +``` + +**Config** — `config/rca.config.json`: `concurrency`, `turnCap`, `softPendingDrain`, +`reaperHeartbeatTtlSec`, `paths.stateDir`, `evidenceRouting`. Read it once at the +gate and pass the values down; a coordinator should never need to open it. + ## Step 0 — input Parse the build id from the invocation args. Accepted forms: a bare build id, a @@ -44,7 +158,76 @@ pass. The gate has two parts; both run before any RCA work starts. ### Part A — connector discovery + validation -Enumerate every connector relevant to test RCA: +**Step 0 — enumerate connector-shaped skills FIRST (before probing raw MCP tools).** +Run: + +```bash +# cwd, the WORKSPACE ROOT above it, and the user dir. The middle one matters: +# when this plugin is itself a repo inside the workspace, cwd is the plugin and +# the product's connector skills sit one or two levels UP, so a bare +# `ls .claude/skills/` finds nothing and the run silently degrades to raw MCP +# tools with best-effort repo guesses — on a real run it missed every +# connector actually present on the workspace this way. +ls .claude/skills/ ../.claude/skills/ ../../.claude/skills/ ~/.claude/skills/ 2>/dev/null +``` + +For each `SKILL.md` found, open it and look for a **Capability declaration** +block (or a `capability: ` line in the frontmatter/body). Any skill that +declares `capability: github | infra | logs | metrics | other` **IS** the +connector for that capability and MUST be added to the manifest — it +**SUPERSEDES** the raw MCP tool for that capability because it carries +product-specific routing (repo map, cluster/namespace, branch conventions, +falsification protocol) the raw tool does not. Record the skill name in the +manifest entry (e.g. `github: valid, via: gh (skill=-github)`). Skipping +this step is the failure mode where the orchestrator dispatches coordinators +that grep the wrong repos on the wrong branch. + +**Disambiguating by product (nudge / one-question rule).** Connector skills are +product-scoped — a workspace may hold none, one, or several product families +(e.g. `-*`, `-*`, whatever the user has). After the `ls`, +pick the _product family_ whose connector skills apply to THIS build: + +**REQUIRED before you open any single family's SKILL.md: list every family the +`ls` output actually returned, one line each, THEN check each one's failure- +signature match — never open just the first (or only) one you happen to +notice and stop there.** This has gone wrong on a real run: the `ls` returned +three families (`a11y-*`, `tm-*`, `tra-*`), the build's actual failures were +accessibility/Workflow-Analyzer domain, and the orchestrator read only +`tra-regression-context` (a TRA/Observability connector whose declared lanes +don't include accessibility at all) — never opened `a11y-regression-context`, +the one that actually matched. That silently produced "exactly one family, use +it" behavior even though three were present, and the mismatch then had to be +patched by asking the user two separate questions Part B says never to ask. +**If you are about to read one family's SKILL.md and cannot recite the other +families the `ls` output also returned, STOP — you skipped the enumeration.** +The failure-signature check (step 2 below) is what catches a family that looks +present but doesn't actually own this build's failures; skipping straight to +one file is exactly how a wrong-family read reaches Part B undetected. + +- **Zero families found** → **nudge the user in the gate summary**: + "No connector-shaped skills found under `.claude/skills/` — proceeding with + raw MCP tools only; culprit-PR attribution will be best-effort against + workspace `git remote` guesses. Add a `-github` / `-infra` + skill for higher-fidelity routing." Then proceed with raw connectors. **Do + NOT block.** +- **Exactly one family** → use it. No question. +- **Multiple families** (e.g. `-*` AND `-*` …) → try to + disambiguate WITHOUT asking: + 1. Match the build's project / build name (from `getBuildId` metadata or + the invocation args) against each family's SKILL.md description / product + hints — if one family matches unambiguously, use it. + 2. Match the discovered failure signatures (from Step 2's `listTestIds` if it + has already run, else defer this to a re-visit after discovery) against + each family's declared file paths / error patterns — if one family owns + the failure surface, use it. + If both signals leave the choice ambiguous, this earns the **one + consolidated gate question** (Part B rules apply): fold it into the same + question as any other non-assumable field, e.g. _"Multiple product families + found (``, ``); build/failure signatures don't + uniquely pick one — which family owns this build's failures?"_ Headless: + pick the first alphabetically and record the ambiguity as a gap. + +Then enumerate every connector relevant to test RCA: - from `config/rca.config.json` → `evidenceRouting`: **github** (product_code/deploy/ci), **infra** (whatever runtime the user has — k8s, @@ -53,15 +236,95 @@ Enumerate every connector relevant to test RCA: - plus any connector-shaped skills / MCP servers present in the session (a log-search MCP, a metrics MCP, an infra skill, …). -**Validate** each with a cheap probe — discovery alone is not enough: +**Validate** each with a cheap probe — discovery alone is not enough. **Every +row below is independent of every other row — fire them all as one batch of +parallel tool calls, never one connector at a time.** A probe failing (or +being absent) never blocks another connector's probe from running; there is +nothing here for one row to wait on. For example: `gh auth status`, `kubectl +version --request-timeout=5s` (or whatever infra tool applies), a logs-MCP +check, and a metrics-MCP check all belong in the SAME turn — not four separate +turns run one after the other, and not "check github, then check infra, +then …". (This is the same class of bug Step 4b hit on a real run: a +sequential-*looking* list of independent checks got executed sequentially in +practice, costing minutes it never needed to. Don't repeat that here, at the +very front of the pipeline where it delays everything downstream.) + +**This rule has already been read and violated on a real run — measured +cost 4+ minutes on gate/scope probes alone.** The coordinator had this exact +paragraph available and still issued `gh api `, an env-var check, a +second env-var check, `kubectl get ns`, `kubectl auth can-i`, `kubectl get +pods` (×2), and `kubectl get pod` as eight separate messages, one Bash call +each, 10-34 seconds apart. Restating the rule again clearly did not prevent +that, so treat it as a hard gate, not a preference: + +- **REQUIRED before your first probe Bash call:** write out the full list of + every probe you are about to run this pass — every base probe, every scope + probe, every target — one line each. Then issue every item on that list as + its own tool-call block **in this one message**. +- **If a message you are about to send contains exactly one Bash call for a + probe, and your list above still has unissued items with no dependency on + that call's result — STOP.** That message is the violation in progress. + Add the rest of the list to it before sending. +- "I'll check github's connector first, then move to infra" is the + rationalization that produced the 4-minute real-run cost above. It sounds + like reasonable sequencing; it is the forbidden pattern. github's probes + and infra's probes have no dependency on each other — there is no "first." +- The only real dependency is per-connector: a connector's scope probes wait + on that SAME connector's base probe, nothing else. Two different + connectors' probes never wait on each other, ever. + +| Connector | Probe | +| --------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| github | `gh auth status` (or a GitHub MCP tool listed) | +| infra | ANY runtime connector the user has — probe what exists, never assume one: `kubectl version --request-timeout=5s`, `docker ps`, `aws ecs list-clusters`, `nomad status`, `pm2 ls`, or an infra-shaped skill/MCP tool. Record the KIND in the manifest (`via: kubectl \| docker \| ecs \| …`) | +| logs | a log-search skill/MCP tool actually listed in the session | +| metrics | a metrics skill/MCP tool actually listed in the session | +| other | best-effort; default `absent` | + +**Scope validation — run the SCOPE PROBES declared by each connector skill.** +The base probe above (`gh auth status`, `kubectl version`, …) only confirms the +raw tool works. It does not confirm the _concrete targets_ a coordinator will +touch — specific repos, branches, clusters, namespaces, indices — are actually +reachable. That is the connector SKILL's job: each connector skill MUST +declare, in its `Capability declaration` section, a `Scope probes:` list +naming what to check and how. This orchestrator's contract is generic: + +1. For every connector skill added to the manifest in Step 0, read its + `Scope probes:` list. +2. **Run every declared probe, across every connector and every target it + names, together in one batch — the same rule as the base probes above.** + The only real ordering constraint is *within* a single connector: its scope + probes are only worth running once that same connector's base probe has + passed (no point checking which repos github can reach if `gh auth status` + already failed). That is a per-connector dependency, not a global one — + e.g. github's repo-scope probes and infra's namespace-scope probes never + depend on each other, so they still fire in the same batch as soon as + their respective base probes clear. Never run one connector's scope + probes, wait for them, then move to the next connector's. +3. Record every target's result in the manifest entry — passes go into a + resolved-scope field (e.g. `repos_validated: [...]`, `namespace: ok`), + failures go into a per-target gap (e.g. `: 404 not_accessible`). +4. A per-target failure is a scoped gap, not a connector-wide failure — the + connector stays `valid` for the targets that did pass. + +Coordinators can then act freely inside the resolved scope and must fail +closed outside it. This closes the failure mode where a coordinator degrades +to `unavailable` because the orchestrator didn't confirm the specific target. -| Connector | Probe | -|---|---| -| github | `gh auth status` (or a GitHub MCP tool listed) | -| infra | ANY runtime connector the user has — probe what exists, never assume one: `kubectl version --request-timeout=5s`, `docker ps`, `aws ecs list-clusters`, `nomad status`, `pm2 ls`, or an infra-shaped skill/MCP tool. Record the KIND in the manifest (`via: kubectl \| docker \| ecs \| …`) | -| logs | a log-search skill/MCP tool actually listed in the session | -| metrics | a metrics skill/MCP tool actually listed in the session | -| other | best-effort; default `absent` | +**A real run skipped this whole section — not one scope probe ran, for a +connector that declares seven of them.** The base probes (`gh auth status`, +`kubectl version`) passed and the run went straight to Step 2's `listTestIds`, +never reading or running the connector's `Scope probes:` list at all. **Before +your first `listTestIds`/discovery call: confirm you can name every scope +probe you ran and its result, for every connector recorded `valid` in the +manifest.** If a connector is `valid` in the manifest and you cannot name a +single scope-probe result for it, STOP — go back and run its declared list (or, +if it genuinely declares none, the manifest-time warning below is the only +legitimate reason to have nothing to name). + +Skills that don't declare `Scope probes:` degrade to a manifest-time warning +("scope probes missing — coordinator may over-degrade"). Do not invent +product-specific probes here. Output the **validated capability manifest**: `connector → valid | invalid | absent` (`lib/routing.mjs` → `buildManifest`; `valid` maps to @@ -81,8 +344,23 @@ is the point: - the current branch for the working branch, - cheap inference (e.g. the automation repo is the cwd if it holds the tests). +**Check the selected connector skill's own intake-defaults section FIRST — before +falling through to inference, and before ever asking.** A connector skill that +declares "Intake defaults for the gate (Part B)" (or equivalent) is telling you +these fields are answerable outright for its product, by build-name/lane or +failure-pattern lookup — not assumptions, not something to ask about. Skipping +straight to inference or to the user when the connector already names the +answer is the exact bug a real run hit: the selected connector's own intake +section explicitly read _"An orchestrator that... asks the consolidated +question about them is reading the wrong place"_, and the run asked anyway — +two separate questions, not even the allowed single one. If the connector's +intake section doesn't resolve a field for THIS build (e.g. its lane table +doesn't match the failure signature at all), that is itself a sign the wrong +family was selected — go back to the enumeration step above before treating +the field as genuinely non-assumable. + **Product-repo corroboration (do NOT skip).** The product repo must plausibly -be the *system under test for THIS build's failures* — not merely a repo name +be the _system under test for THIS build's failures_ — not merely a repo name found lying around. A repo mentioned only in workspace docs/READMEs is a **weak hint, never an assumption**: cross-check it against the failure signatures (discovery runs first if needed) — do the failing area, files, or error strings @@ -92,6 +370,7 @@ the doc-sourced repo is discarded — never carry it (or its PRs) into the manifest as a settled product repo. When corroboration leaves **no** product repo, decide by whether a human can help: + - **PRs were supplied** → treat those as the suspect surface; product repo is derived from them. No question needed. - **No PRs, interactive session** → the product repo is now **non-assumable AND @@ -109,9 +388,22 @@ non-assumable AND load-bearing. In practice that set is: the build id; **the product repo when it could not be corroborated and no PRs were supplied** (see above — without it the culprit-PR hunt cannot run); and rarely an ambiguous repo when PRs were supplied. Those, and only those, may be asked **ONCE, in a single -consolidated question at gate close** — e.g. *"Failures look like ``; +consolidated question at gate close** — e.g. _"Failures look like ``; which repo owns that code? (reply 'none' → I'll RCA without culprit-PR -attribution)."* Never a second question. **Headless: skip asking entirely; +attribution)."_ Never a second question. + +**This has already been violated on a real run** — a product-family +disambiguation question and a repo-ownership question went out as two separate +`AskUserQuestion` calls, 33 seconds apart, instead of one consolidated +question (or, better, no question at all, since the connector's own intake +defaults answered both — see above). **Before your first `AskUserQuestion` +call this pass: write out every field this run still needs from the user, +across every reason it might be non-assumable, in one list — then ask them as +ONE question with multiple parts if more than one survives.** If you are about +to send a second `AskUserQuestion` call in the same gate pass, STOP — fold its +content into the first question instead, or if the first has already been +sent, that is the violation; there is no second gate question, ever. +**Headless: skip asking entirely; record the gaps.** ### Gate close @@ -135,6 +427,20 @@ listTestIds(buildId=, status="failed", includeFailureDetail=true) (`failure.{category, error_summary, file_path, …}`) — the seed for clustering, so no per-test probe turns are needed. +**First, sweep the state directory** (`lib/state-dir.mjs` → `hardenStateDir(dir)`). +Per-write hardening only tightens the file being written, so artifacts from a +build analysed before that landed keep their old permissions forever — a +completed build is never rewritten. Found in practice: the directory itself was +`drwxr-xr-x` with six `0644` files inside, holding root causes, culprit PRs and +log excerpts in a shared OS temp dir. The sweep is cheap and idempotent, so run +it unconditionally; it never throws, skipping anything it cannot chmod. + +Nothing deletes these artifacts when a run finishes, and that is deliberate — +resume is keyed on `buildId` → same path, so cleaning up on completion would +break `pending-resume`. `pruneStateDir(dir, nowMs)` exists for growth (default +7 days, far longer than any run) but is **not** automatic: these files *are* the +resume state. Call it explicitly, with `dryRun: true` first. + Resolve the state file with `lib/csv-state.mjs` → `csvPathFor(buildId, config.paths.stateDir)` — the **build id is in the filename** and the default directory is **OS temp** (`/bstack-rca/rca-state..csv`), so @@ -147,45 +453,634 @@ Re-running `seed` on an existing CSV is idempotent and preserves terminal rows (resume-safe — same build id → same path). If `listTestIds` returns empty → write an empty CSV, report "no failed tests", stop. -## Step 3 — failure-signature clustering (see references/clustering.md) +## Step 3 — clustering (see `/skills/rca-build/references/clustering.md`) -Compute a failure signature per row and assign `cluster_id` (`lib/signature.mjs`). Each cluster gets one **representative** (full multi-turn loop) and `N−1` **siblings** (pre-seeded one-turn confirm against their own logs). This collapses the expensive evidence hunt to O(distinct causes) while every test still lands a per-test RCA. Singleton clusters are just plain per-test loops. -## Step 4 — build-evidence pre-compute (see references/evidence-routing.md) +**Prefer the server's own clustering over recomputing it client-side.** A real +run skipped straight to the client-side fallback below without ever calling +`getBuildFailureThemes` — the tool's schema had even been loaded via +`ToolSearch` that pass, it was simply never invoked. **`clusterAndPersist` may +ONLY be called after a `getBuildFailureThemes` call this pass returned +`ready: false` (or errored) — never as a first move.** If you are about to call +`clusterAndPersist` and cannot point to this pass's own `getBuildFailureThemes` +call and its `ready: false` result, STOP — you are taking the fallback without +ever having tried the preferred path, which throws away the server's own +root-cause grouping for no reason and degrades every run to text-signature +clustering by default instead of by necessity. -Once, before fan-out (the capability manifest already exists from Gate Part A — -reuse it, do not re-discover): +1. Call `getBuildFailureThemes(buildUuid=)`. If nothing has ever + been computed for this build, this triggers computation (one POST, same + call) and polls in-call up to its own budget for `buildThemeWorkflow.status` + to reach `SUCCESS`. +2. **`ready: true`** → for each entry in `buildThemes`, call + `listTestsInFailureTheme(buildUuid=, themeId=)`, + following `nextCursor` until exhausted, to get that theme's member + testRunIds. Feed rows + the themes result + the per-theme member lists into + `lib/theme-clustering.mjs` → `clustersFromThemes(rows, themesResult, + testsByThemeId)` — this is the **preferred path**, since the grouping + reflects the server's own root-cause analysis rather than a text-signature + guess, and it never runs the coordinator fan-out N-tests-wide for a build + with only a handful of distinct causes duplicated across teams. Any failed + test the server didn't assign to a theme is never dropped — it still gets + its own singleton cluster. -- **Build-level evidence** — compute the last-green→this-build delta (diff, - deploy timeline, suspect-PR window) **once** and pre-seed every coordinator - with the same grounded window. Cache by `(repo, commit-range)`. No "last green" - baseline (never-green suite) → fall back to a configured baseline ref and log it. + **`rows` MUST be `readRows(csvPath)` — the CSV Step 2 already seeded — + never a `listTestIds` result variable held over from earlier in the turn.** + A real run hit exactly this: an earlier `listTestIds(status="failed")` call + in the same session errored ("fetch failed"), a later call used a + *different* status filter, and `clustersFromThemes` was fed whatever `rows` + was still in scope — every theme member came back unmatched (every + `rowById.get(...)` lookup missed), which reads exactly like a "test ID + mismatch" but isn't one: `getBuildFailureThemes`/`listTestsInFailureTheme` + themselves returned correct data the whole time. The result: the CSV ended + up with signature-hash `c-xxxxx` cluster IDs (`clusterAndPersist`'s fallback + format) instead of `theme-`/`solo-`, i.e. the preferred path was + silently abandoned even though it never actually failed. The CSV is the one + row set guaranteed fresh and from a successful seed (Step 2 only seeds + after `listTestIds` succeeds) — always re-read it here rather than trusting + a variable carried over from turns ago. +3. **`ready: false`** (still computing past the poll budget, a failure status, + or `status: "trigger-unavailable"` — the trigger call didn't succeed) → + **fall back** to **`clusterAndPersist(csvPath, csvStateModule)`** + (`lib/signature.mjs`), not `clusterRows` directly: + + ```js + const clusters = clusterAndPersist(csvPath, await import("./lib/csv-state.mjs")); + ``` + + `clusterRows` assigns `cluster_id` **in place** and returns `{rows, clusters}`, + so `const { clusters } = clusterRows(rows)` gives you working cluster objects + while every `cluster_id` is silently discarded — the CSV keeps empty cluster + columns and the run degrades to **one coordinator per test**, losing the whole + representative/sibling collapse. This is a real failure mode, not a + theoretical one — a real run hit it, with the clustering silently "done" in + the return value but never written to the CSV. `clusterAndPersist` writes + back and verifies the count, so it cannot forget. + + Never block the run waiting on the server; the fallback keeps the same + `{ cluster_id, representative, siblings }` contract the fan-out consumes, + so nothing downstream needs to know which path produced it. This also + makes the flow independent of whether the trigger call currently succeeds: + whenever it doesn't, `getBuildFailureThemes` degrades to `ready: false` + fast (no wasted poll budget) and this fallback engages every time; + whenever it does, the same call reaches `ready: true` on its own and this + fallback simply isn't exercised — no code change required either way. + +`clustersFromThemes` mutates each row's `cluster_id` in place but does NOT +persist — it's pure/dependency-free by design. Write its rows back yourself +with `csvState.writeRows(csvPath, rows)` before fan-out; `clusterAndPersist` +already does this for the fallback path. Then verify either way: **if +`cluster_id` is empty on any row, Step 3 did not take effect** — do not +proceed, the run would silently cost O(tests) instead of O(causes). + +## Step 4 — build-evidence pre-fetch (see `/skills/rca-build/references/evidence-routing.md` and `/lib/evidence-file.mjs`) + +Once, after clustering (Step 3) and before fan-out — the capability manifest +already exists from Gate Part A, reuse it, do not re-discover. This step +replaces each coordinator's own turn-1 evidence sweep with ONE pre-fetch: +it does not remove the requirement that turn-1 evidence exists, only _who +gathers it_. + +**Narrate this as one combined phase, not two sequential ones.** Step 4b +(below) starts the moment Step 3 finishes and runs the whole time Step 4 does +— any progress line shown to the user during this window should say something +like `Evidence pre-fetch (Step 4) + turn-1 pre-dispatch (Step 4b)`, never "Step +4 done, now starting Step 4b." That sequential phrasing is exactly what caused +Step 4b to be *executed* sequentially in practice on a real run — the +narration and the execution went wrong together, and fixing only one of them +leaves the other free to reintroduce the bug. + +1. Resolve the evidence-file path: `lib/evidence-file.mjs` → + `evidencePathFor(buildId, config.paths.stateDir)` — + `/bstack-rca/rca-evidence..json`, alongside the state CSV. + `initEvidenceFile(path, buildId, nowMs)`. +2. **Scope the pre-fetch to the full union, never a single guess:** + - **Repos** — every repo in Gate Part A's scope-probe-validated + `repos_validated` list (e.g. a VRT-lane build validates `frontend` + + `railsApp`; an nl2steps build validates `misc-services` + `ai-sdk-node`). + - **Workloads** — the union of workloads every cluster's **representative** + implicates, via the active connector skill's failure-signature→workload + routing table (never one workload guessed from the first failing test). +3. For each repo: run the connector skill's PR-window-search + deploy-state + recipes **once**, using `lib/evidence-cache.mjs`'s `compute(repo, range, +evidenceType, fn)` to dedupe if two steps need the same `(repo, range)`. + Digest the result into the `evidence-block.md` shape, then persist via + `setGithubEvidence(path, repo, {deployState, prsInWindow, gap}, nowMs)`. + A repo the connector can't reach records `{gap: ""}` — never blocks + the rest of the pre-fetch. + + **Every repo's PR-window search is independent of every other repo's — + fire all of them as parallel tool calls in ONE message, never one repo, + read its result, then the next repo.** The same rule that governs Gate + Part A's connector probes applies here at repo granularity: write the + full repo list from step 2 first, then issue every repo's `gh pr list` + call together. A message containing exactly one repo's fetch, with other + repos from the union still unfetched and no dependency on this one's + result, is the violation — go back and batch the rest in before sending. + + **`--json` on THIS FIRST `gh pr list` call MUST include `files` — there is + no separate step where it gets added later.** This is the single + highest-leverage thing in Step 4, and it is a MUST, not a nice-to-have: a + PR-list call than omits `files` here is never corrected downstream — it + just becomes one `gh pr view --json files` per PR, run from inside the + `for pr in ...` loop this exact mistake produces. This has happened on a + real run: the orchestrator listed PRs without `files`, then looped `gh pr + view --json files` once per PR to backfill it — entirely avoidable had the + first call carried `files`. There is no legitimate reason to split these + into two calls; `--json files` costs nothing extra on the list call itself. + + ```bash + gh pr list -R / --state merged --base \ + --search 'merged:..' --json number,title,mergedAt,url,files --limit 100 + ``` + + `--json files` returns every PR's changed paths in the SAME call, so one + request per repo replaces one `gh pr view --json files` per PR across + every coordinator. Across real runs, per-PR file-list fetches have been a + meaningful slice of all `gh` traffic — entirely avoidable here. + Store the paths in each PR's `files` field rather than leaving it `null`: + path-overlap is the first falsification test in + `/skills/rca-build/references/github-evidence.md`, so with `files` populated a coordinator + rules a suspect in or out from the evidence file alone, and only fetches a + diff for the handful that survive. Do NOT pre-fetch diffs — those are large + and only a few PRs ever need one. + + A per-PR `gh pr view --json files` call is legitimate ONLY for a suspect + PR discovered later (during a coordinator's own investigation, not in this + pre-fetch's window) — never as a backfill for a PR-list call that should + have carried `files` the first time. + + Two other real wastes this step should pre-empt: + - **Never let coordinators re-probe connectors.** `gh auth status` / + `kubectl version` calls have shown up repeatedly from coordinators purely + because the manifest wasn't trusted. State plainly in the dispatch prompt + that the gate validated them. + - **File contents are a large, only partly predictable slice of `gh` + traffic**, so do NOT bulk-fetch them. The `files` lists above tell a + coordinator exactly which files matter, and the tool cache dedupes the + ones two coordinators both open. +4. For each workload: run the connector skill's compulsory kubectl + + VictoriaLogs sweep **once**, anchored to the build's own clock — never + "now". **Every workload's sweep is independent of every other workload's + and of every repo's fetch in step 3 — batch all of them into the same + message(s), same rule as step 3's repo fetches.** **PAD the window: + `started_at − 2m` .. `finished_at + 10m`.** + `finished_at` is when the build was _marked_ finished, which is not when + the failing behaviour stopped: on a real build, an upstream outage was + still ongoing after `finished_at` was recorded — a sweep scoped strictly + to `started_at..finished_at` would have caught only the very start of it + and missed the cause entirely. Label every + finding with whether it falls inside or outside the strict window so a + coordinator can weigh it; do NOT silently widen to an arbitrary window + (that is the separate, opposite failure of matching a coincidence from + unrelated traffic). Persist via `setLogsEvidence(path, workload, +{clusterIds, kubectlSweep, victorialogs, gap}, nowMs)`. + + Two query mechanics that cost real calls when missed: + - **`direction` defaults to newest-first**, so a limited query always + returns the END of the window. To find when something _started_ — the + first request after a gap, the onset of an error burst — pass + `direction: "forward"`. A gap "confirmed" from a backward query is not + confirmed at all; it is just the tail of the range. + - **Absence needs a control.** A zero-result query is indistinguishable + from a wrong selector. Before reporting "no traffic", prove the logger + was alive in the same window with a query you expect to be non-empty + (e.g. readiness probes from a named pod). Only then is silence evidence. + +5. `resolveBaseline(lastGreenRef, fallbackRef)` (from `lib/evidence-cache.mjs`) + → `setBaseline(path, baseline, suspectWindow, nowMs)`. No "last green" + baseline (never-green suite) → fall back to a configured baseline ref and + note the weaker grounding — this note travels into the file, not just a + spoken log line, so every coordinator sees it. +6. **Resolve local clones ONCE** (`lib/repo-source.mjs`). File *contents* are + the largest remaining slice of github traffic, and most of it can be served + with no network at all when the machine already has the repos checked + out — a local `git show` returns the same bytes as `gh api` far faster, + with no round trip. + + ```js + const d = discoverWorkspaceRoot({ repos: reposValidated, from: pluginRoot }); + const { pins } = deployShas(path); // structured, not prose + const localRepos = d.root + ? resolveLocalRepos({ repos: reposValidated, pins, workspaceRoot: d.root }) + : {}; + setLocalRepos(path, { workspaceRoot: d.root, repos: localRepos }, nowMs); + ``` + + `discoverWorkspaceRoot` takes the **validated repo list** and accepts a + candidate directory only if it actually contains one of *this run's* repos — + that check is what keeps the plugin generic, and it is bounded to ~3 tries + because guessing harder risks reading an unrelated checkout, which is + silently wrong rather than merely slow. Finding nothing is a fine outcome: + every read falls back to the cached `gh` path. + + Set `deployState.sha` explicitly when you write each repo's entry. + `deployShas()` falls back to parsing the prose `summary`, but that is a + safety net, not the contract: when the wording drifts it returns an empty + map, and every read silently degrades to the network while still looking + like it worked. + + `pins` must be the **build-time commit shas** from `deployState`, never + branch names. A developer's clone is routinely stale, and reading a branch + locally has returned different bytes than the real head — for RCA that is + a confident wrong answer about code that never shipped. + + Doing this at the gate is the point: every coordinator then reads a map + instead of probing the filesystem itself. +7. `recomputeCoverage(path, {repos, workloads}, nowMs)` and declare the + resulting path in the gate summary alongside the capability manifest, so + a human re-reading the run can find it. + +**Size discipline is enforced at write time, not just at submit time.** Every +leaf (`deployState`, each PR, each log sweep) must already be a digested +`block` per `evidence-routing.md`'s caps (`SUMMARY≤80`, `SNIPPET≤4/8 lines`, +link over diff) — never a raw dump. Cap `prsInWindow` to the top ~30 candidates +by path-overlap relevance, not every PR in the window. + +Pass `evidencePathFor(...)`'s path to Step 5's fan-out as `evidenceFilePath` — +every dispatch (representative and sibling) must be told to read it first. + +## Step 4b — turn-1 pre-dispatch (fire-and-forget, fully async alongside Step 4) + +Every cluster's representative testRunId is already known the moment Step 3 +finishes, for however many clusters this build produced — never assume a +fixed count, it is whatever Step 3 found. Turn 1's message has no dependency +on Step 4's evidence pre-fetch at all: it is built entirely from Step 2's CSV +seed (`error_summary`/`testName`), exactly the same construction +`agents/ai-tfa-coordinator.md`'s loop step 0 uses when neither `pre_seed` nor +`resume` applies (`error_digest` present → `"Error: "`; else +→ `"Initiating collaborative RCA for test run <id>."`). So there is no need to +wait for Step 4 before starting Step 4b — and, just as importantly, no need to +wait for Step 4b either before moving on. + +**Mechanic: dispatch, don't wait.** For every cluster representative, launch +one lightweight subagent via the Agent tool whose ONLY job is to call +`tfaRcaTurn(testRunId=<rep>, message=<first-turn digest>)` once and emit one +fixed-shape block as its final output — no evidence gathering, no loop, no +drain. This is deliberately **not** a full `ai-tfa-coordinator` dispatch (that +agent's whole design is the multi-turn evidence-gathering loop, far more +machinery than "submit one message and return"); write a minimal, +purpose-built inline prompt for this instead, and put the exact output +contract below directly in that prompt — an Agent-tool result is free text, +and with many of these dispatched concurrently the orchestrator has no other +reliable way to tell which representative a given notification is even for. + +``` +TURN1_OUTPUT_START +testRunId: <the testRunId this subagent was given> +status: RESOLVED | NEEDS_INFO | PENDING +threadId: <threadId from the tfaRcaTurn response, or "none"> +turnId: <turnId — PENDING only, tfaRcaTurn never returns one for the other two statuses; else "none"> +glimpse: <RESOLVED only — the trimmed {root_cause, failure_type, related_prs, confidence, viewRca} object, verbatim; else "none"> +asks: <NEEDS_INFO only — the asks array, verbatim; else "none"> +TURN1_OUTPUT_END +``` + +That block — not prose, not a summary — is this subagent's entire final +message. It is exactly what the orchestrator reads back off the +task-notification to do the bookkeeping below: `status` selects the branch, +`testRunId` is the join key back to the right CSV row / registry entry, and +`threadId`/`turnId`/`glimpse`/`asks` are pasted straight into `flip()` or +`recordTurn1()` with no re-interpretation needed. + +An Agent-tool dispatch returns *immediately* with a launch confirmation, not +the subagent's result — this is fundamentally different from a batch of raw +MCP tool calls in one turn, which blocks the orchestrator until every call in +that turn returns. Fire off every representative's dispatch together, then +**immediately proceed to Step 4's evidence pre-fetch in the very next turn — +do not wait for any of them.** There is no "same batch as Step 4" trick to get +right here (an earlier version of this section relied on that and it is easy +to execute wrong, e.g. by finishing Step 4 first and only then starting Step +4b — the fire-and-forget dispatch here has no such ordering hazard, because +nothing about it requires being co-located with Step 4's own tool calls). + +As each subagent finishes — on its own schedule, bounded only by +`tfaRcaTurn`'s own ~90s in-call poll cap, so realistically within the first +minute or two of the run — a task-notification carrying its `TURN1_OUTPUT` +block arrives, interleaved with whichever Step 4 turn happens to be in flight +at that moment. Handle each one the moment you are next free to, as pure +bookkeeping — no new tool calls needed for this part: + +1. `initTurn1Registry(turn1PathFor(buildId, config.paths.stateDir), buildId, nowMs)` + once, before dispatching any turn 1s (`lib/turn1-registry.mjs`). +2. **Skip any representative whose CSV row already has a `threadId` + + `turnId`** (a `pending-resume` row from a prior run attempt — an already + in-flight thread). Dispatching a fresh turn 1 for it would start a SECOND + thread for the same test, which every other part of this contract + (`agents/ai-tfa-coordinator.md`'s "one thread per test" hard limit) forbids. + That representative resumes its existing thread at Step 5 exactly as + before Step 4b existed — Step 4b only ever applies to a representative with + no prior thread at all. +3. For every remaining (thread-less) cluster representative, dispatch its + turn-1 subagent. When its result notification lands, branch on it: + - **RESOLVED** → `flip()` this CSV row straight to terminal, right here — + same fields a coordinator's `RCA_OUTPUT` would set (`rca_done: resolved`, + `root_cause`, `failure_type`, `related_prs`, `view_rca`, `confidence`, + `turns_used: 1`, `threadId`). This representative needs **no Step 5 + dispatch at all** — the cheapest possible outcome. **Do not wait for + Step 5 to formally start: dispatch this cluster's siblings immediately, + right here in Step 4b** — as their own fire-and-forget Agent-tool + dispatches too, same principle, don't wait on them either — via + `siblingPreSeed(csvPath, csvState, clusterId, representativeId)` against + the row you just flipped. A sibling only ever needs its OWN + representative's result, never the state of any other cluster, so + nothing about Step 5's fan-out has to begin first. This is the ONLY case + a sibling can be dispatched this early, and the reason is narrow: it + works because the representative resolved in ONE pre-dispatched turn, so + `pre_seed` is already real evidence, not a guess. A representative still + mid-loop (`NEEDS_INFO`/`PENDING`) has no `root_cause` yet — dispatching + that cluster's siblings before it lands would degrade every one of them + into a full independent investigation, at real representative-level cost + instead of a cheap one-turn confirm (see Step 5's sibling-ordering note). + Never do that; siblings of a not-yet-resolved representative wait for + Step 5 exactly as documented there. + - **NEEDS_INFO** → `recordTurn1(path, testRunId, {status: "NEEDS_INFO", + threadId, asks}, nowMs)`. A real, non-terminal answer — hand it to Step + 5's coordinator as `turn1_result` (never resubmit turn 1). + - **PENDING** → `recordTurn1(path, testRunId, {status: "PENDING", threadId, + turnId}, nowMs)`. Do **not** drain it here — there is no reason to spend + any of the orchestrator's own time on it. Step 5's coordinator dispatch + already knows how to drain a soft-PENDING (the existing `resume` input + covers this case as-is). +4. Nothing about this starts a second thread: it is exactly turn 1 of the one + thread the Step 5 coordinator continues from `threadId`. +5. **A subagent that never reports back fails open, not closed.** If a turn-1 + subagent dies, errors, or times out before emitting its `TURN1_OUTPUT` + block, no registry entry gets recorded for that representative — there is + nothing to distinguish "Step 4b never ran for this test" from "Step 4b ran + and failed." Both land in exactly the same place: Step 5's `readTurn1` + returns nothing, and Step 5 falls back to a completely normal, fresh + dispatch (submit turn 1 from scratch, no `resume`/`turn1_result`) — which + is functionally the retry. There is no separate "check Step 4b succeeded, + re-trigger turn 1 if not" step to build; the existing no-entry fallback + already covers it. The one real cost: if the dead subagent *did* reach + `tfaRcaTurn` before failing to report back, that thread is now orphaned — + Step 5's fresh dispatch starts a genuinely new thread rather than resuming + it. Not a correctness problem (the new thread resolves independently just + fine) — just one wasted, never-continued thread on TFA's side per failure. + +**This removes orchestrator-side blocking, not underlying capacity — cap the +fan-out itself.** Every dispatched subagent still makes a real `tfaRcaTurn` +call, consuming the same API/compute capacity Step 5's fan-out competes for. +"Async" means the orchestrator never sits idle waiting on these dispatches — +it does NOT mean the dispatches are free, and firing an unbounded number of +them at once for a build with many clusters risks the same session/rate-limit +cascade a large Step 5 fan-out can hit. **Dispatch at most `concurrency` (from +`config/rca.config.json` — the same value Step 5 already uses, not a separate +setting) turn-1 subagents at a time.** For a build with more cluster +representatives than that, issue the first `concurrency` immediately, then +issue the next batch as soon as they're dispatched (still fire-and-forget, +still never blocking Step 4's own progress) rather than firing every +representative in one shot regardless of cluster count. + +None of this — `initTurn1Registry`, the pending-resume skip-list check, or the +first dispatch batch — has any dependency on Step 4's own tool calls, or vice +versa. **The very first turn can contain Step 4b's setup-and-first-dispatch- +batch together with Step 4's own first evidence-gathering calls, in the same +batch.** Do not treat Step 4b's prep as a turn Step 4 waits behind, even for +one turn — that is the same one-extra-turn-of-latency mistake this whole +section exists to remove, just smaller. + +Pass `turn1PathFor(...)`'s path to Step 5 alongside `evidenceFilePath` — Step 5 +must read it (`readTurn1(path, testRunId)`) before building each +representative's dispatch and translate the result into the matching input: +`PENDING` → `resume: {threadId, turnId}`; `NEEDS_INFO` → `turn1_result: +{threadId, asks}`; a flipped-to-terminal row (no registry entry, CSV already +`resolved`) → no dispatch, use the CSV row's result directly as this cluster's +representative outcome for seeding siblings. ## Step 5 — fan-out (fully autonomous) -Drive the cluster work-list, **`concurrency` (default 5) at a time**: +**REQUIRED gate before your first Step 5 dispatch: Step 4b's dispatch batch +must have already been ISSUED this pass — not completed, not waited on, +issued.** A real run skipped Step 4b entirely — no lightweight turn-1 +pre-dispatch subagent was ever launched, and all N cluster representatives +went straight to a full `ai-tfa-coordinator` dispatch here instead, paying +full multi-turn coordinator cost for every cluster including the ones that +would have resolved in one pre-dispatched turn. **If you are about to issue +Step 5's representative dispatches and cannot point to this pass's +`initTurn1Registry` call and a turn-1 dispatch batch issued for every +thread-less cluster representative, STOP — go back and fire that dispatch +batch first.** This gate is about the dispatch having gone out, same +fire-and-forget contract Step 4b already documents — it is NOT a "wait for +Step 4b's subagents to finish" gate, and reading it that way reintroduces the +exact sequential-latency bug Step 4b exists to remove. In practice this batch +should already be long since fired by the time you reach Step 5, since Step +4b's own instructions have it go out in the same turn as Step 4's first +evidence-gathering calls — this check exists only to catch the case where +that never happened at all, not to insert a new wait. + +**ORDER MATTERS: representative first, siblings only after it lands.** For each +cluster, dispatch the representative, wait for its row to go terminal, then +dispatch its siblings carrying `pre_seed` from +`siblingPreSeed(csvPath, csvState, clusterId, representativeId)`. Clusters are +independent, so they still run concurrently *with each other* — the barrier is +per cluster, not global. + +A sibling is only cheap because it confirms a hypothesis someone else already +established. Dispatch one without that hypothesis and "one-turn confirm" +degenerates into a full independent investigation *with the sibling framing on +top*, so it costs MORE than the representative it was meant to be a fraction +of — this has happened on a real run, with siblings running well past +representative-level cost because nothing ordered them after their rep and +nothing refused to dispatch without a seed. It degrades silently, with no +error to flag it. + +`siblingPreSeed` returns `{ok:false, reason}` when the representative is not +resolved or recorded no `root_cause` — **do not dispatch that sibling yet**. +Never hand-roll the seed: the guard is the only thing standing between a +clustered run and O(tests) cost. + +Drive the cluster work-list, **`concurrency` (default 20) at a time**: representatives deep, siblings one-turn-confirm. Eagerly persist to the CSV/WAL (claim → heartbeat → flip) so the run is resumable. -- Claude Code → run the dynamic workflow `workflows/rca-batch.mjs` - (script-orchestrated; gap → "unavailable" back to TFA → best-effort finalize). -- Hosts without the Workflow runtime → dispatch `tfa-rca:ai-tfa-coordinator` - subagents ≤ `concurrency` at a time, or drive the sequential harness - `lib/loop.mjs` (`runRcaLoop`). Same contract, same no-prompt rule. +**"Per cluster, not global" is a rolling work-queue, not two rigid phases.** +Do NOT dispatch "all representatives first, then all siblings" as two fixed +mega-batches — that reintroduces a global-ish wait: any cluster's siblings +would sit idle until every representative in the current batch lands, not just +their own. Instead, whenever a batch of dispatches returns, immediately refill +the next batch by mixing (a) siblings of whichever representatives just +resolved (via `siblingPreSeed`) with (b) any not-yet-dispatched representatives +from other clusters, up to `concurrency` slots — so a fast cluster's siblings +enter the very next batch instead of waiting out an unrelated slow +representative. + +This distinction matters differently on each path: +- **Opt-in `workflows/rca-batch.mjs`** achieves this structurally, for free: + `pipeline(clusters, repStage, siblingStage)` has NO barrier between stages — + a cluster's siblings start the instant ITS OWN representative resolves, + fully interleaved with every other cluster's progress. Nothing to get wrong + here. +- **Default direct Agent-tool dispatch** cannot be sub-batch-streaming the same + way, because a single assistant turn's parallel tool calls are a real + synchronization point: the orchestrator does not regain control until every + call in that turn's batch has returned. So within any one batch, a cluster + whose representative resolves early still cannot dispatch its siblings until + the WHOLE batch drains — the rolling-refill discipline above is what keeps + that batch-local wait from becoming a build-wide one, but it cannot eliminate + it entirely. **When cluster count exceeds `concurrency`, or when the + Workflow tool is available, prefer `workflows/rca-batch.mjs`** for + latency-sensitive builds — it is the only path with a true per-cluster (not + per-batch) guarantee. + +> **Concurrency comes from `config/rca.config.json` — always read it from +> there, never hardcode.** The default path (direct Agent-tool dispatch) honors +> the JSON value literally: fan out coordinator subagents in batches of +> `concurrency` (one message, up to `concurrency` tool-use blocks per batch). +> The opt-in `workflows/rca-batch.mjs` path is subject to the Workflow tool's +> architectural cap of `min(16, cpu cores - 2)` — on that path `concurrency` +> is a soft upper bound and excess work queues rather than running N-wide. +> If you need literal fan-out, use the default direct-dispatch path. + +- **Default (all hosts, including Claude Code) → direct Agent-tool dispatch.** + Read `concurrency` from `config/rca.config.json` and dispatch + `tfa-rca:ai-tfa-coordinator` subagents in batches of that size (one message, + up to `concurrency` tool-use blocks per batch), refilling each next batch per + the rolling work-queue discipline above — never two rigid all-reps / + all-siblings phases. This path is **outside the Workflow runtime**, so the + `min(16, cores-2)` ceiling does not apply and the JSON value is honored + literally. Prefer this path whenever the machine's Workflow cap (`min(16, + cores-2)`) would be smaller than the configured `concurrency` — e.g. an + 8-core Mac caps Workflow at 6 while the JSON asks for 20 — but remember it + only gets per-BATCH streaming, not per-cluster: prefer + `workflows/rca-batch.mjs` instead whenever cluster count exceeds + `concurrency` and the Workflow tool is available. + + **This path has no code enforcing the Step 4b handoff — you are the + enforcement.** Unlike `workflows/rca-batch.mjs` (which reads the registry in + code via `turn1Line()`) and `lib/loop.mjs` (which takes `turn1Result` as a + structural parameter), building a representative's dispatch prompt here is + entirely on you. **Before dispatching ANY representative, call + `readTurn1(turn1PathFor(buildId, stateDir), testRunId)` and fold the result + into the prompt using this exact mapping — the two are distinct coordinator + inputs (`agents/ai-tfa-coordinator.md`), never interchangeable:** + `PENDING` → `resume: {threadId, turnId}`; `NEEDS_INFO` → `turn1_result: + {threadId, asks}`; no registry entry with the CSV row already `resolved` → + skip the dispatch entirely, use the CSV row's result directly. Do NOT fold a + `NEEDS_INFO` result into a `resume` field, or vice versa — a coordinator + reads these as two different shapes and a swapped one is silently wrong, not + rejected. Omit this translation altogether and Step 4b's pre-dispatch is + silently wasted: the coordinator submits turn 1 again on a brand-new thread, + abandoning the one Step 4b already started (not incorrect — the run still + resolves — just the entire latency win thrown away without any error to + notice it by). +- Opt-in `workflows/rca-batch.mjs` (Claude Code only) → use only when the + Workflow tool's structured `pipeline()`/`parallel()` orchestration, + `resumeFromRunId` resumability, or progress UI is worth the concurrency + trade. On this path `concurrency` is a soft target only — the runtime hard- + caps at `min(16, cores-2)` regardless of the JSON value. +- Hosts without the Workflow runtime and without Agent-tool fan-out → drive + the sequential harness `lib/loop.mjs` (`runRcaLoop`) one test at a time. + Same contract, same no-prompt rule. Subagents/coordinators return compact `RCA_OUTPUT` blocks, never transcripts. A coordinator that dies becomes a recorded `failed` row — one stuck test never sinks the batch (partial-first). No path ever prompts the user (the gate is closed). +**Coordinator prompts MUST carry `pluginRoot` and use it to fully qualify every +reference-doc / lib path.** A coordinator is dispatched fresh, with no +guarantee about its own cwd — `references/evidence-routing.md` (bare, +relative) resolves against whatever directory the coordinator happens to +start in, which is routinely NOT this plugin's root. This has cost real +coordinators repeated `Read` attempts at the wrong bare path followed by a +`find` to recover the real one (`<pluginRoot>/skills/rca-build/references/evidence-routing.md`, +`.../github-evidence.md`, `.../clustering.md`). Every dispatch prompt must +state `pluginRoot=<absolute path>` up front and every reference-doc pointer in +the prompt (and echoed from `agents/ai-tfa-coordinator.md`) must already be +`pluginRoot`-qualified — never a bare `references/<file>.md`. + +**Coordinator prompts MUST also point at the API reference instead of letting +the coordinator re-derive it.** State plainly in the dispatch prompt: "Function +signatures for `lib/*.mjs` are documented at `<pluginRoot>/skills/rca-build/SKILL.md` +§ API reference — read that section once if a signature is needed; do not +`grep`/`Read`/`cat` the `lib/` source to re-derive a signature already +documented there." This is a real, recurring self-discovery tax — one +coordinator re-read `lib/evidence-file.mjs` plus a `grep`, all to re-learn +`contributeLogsEvidence`'s signature — a cost this pointer removes. + +**Coordinator prompts MUST name every connector-shaped skill on the manifest.** +Each dispatch prompt lists, per capability, the resolved connector skill from +Gate Part A Step 0 — e.g. _"Use `<resolved-github-skill>` for every +product_code / deploy / ci ask (canonical repos + branch live in the skill; do +NOT grep other repos). Use `<resolved-infra-skill>` for every infra ask."_ A +coordinator prompt that omits a manifest-listed connector skill — and that +therefore lets the +coordinator infer repos from workspace `git remote` or cwd — is a bug: the +coordinator will land plausible-but-wrong PR attributions on adjacent repos. + +**Coordinator prompts MUST also name the Step 4 evidence file.** Every +dispatch prompt (representative and sibling alike) includes the absolute +`evidenceFilePath` from Step 4 with the instruction: _"Read `<path>` (via the +Read tool) before making any live github/infra/logs gather call. It's a +pre-fetch, not a hard dependency — a repo/workload it doesn't name, or marks +with a `gap`, is a genuine gap: fall back to the capability manifest above +exactly as if no file existed."_ For a sibling, add: _"The file's data about +your OWN test's workload is real evidence, not inheritance — reading it is +fine. What must stay independent is the CONFIRMATION judgment: never adopt the +representative's verdict just because the file already has the answer in +it."_ A dispatch prompt that omits this path forces its coordinator back into +a full independent sweep — exactly the redundancy Step 4 exists to remove. + +**The file is read-write, not just read-only.** When a coordinator has to +gather live (a genuine gap), tell it to write the result back — +`contributeGithubEvidence`/`contributeLogsEvidence` (`lib/evidence-file.mjs`), +passing its own `testRunId` as `writerId` — before finishing, not just answer +TFA and move on. A representative's deep dive (a full diff, a downstream +trace, a PR the pre-fetch never named) then benefits its own siblings and any +other cluster sharing the same repo/workload, instead of every one of them +re-running the same live search. This is already baked into +`agents/ai-tfa-coordinator.md`'s Operating Principle 0 for any dispatch of +that agent type — no need to repeat the mechanics in the prompt, just don't +omit `evidenceFilePath` (above), since write-back has nothing to write to +without it. + +**Pre-seed the MCP cache with the queries you just ran.** Step 4's log sweeps +are MCP calls, and a coordinator will often want the same ones. Deposit each +result under the key it would compute — `mcpCacheKey(tool, args)` then +`cachePut(toolCacheDirFor(buildId), key, {…, writerId: "orchestrator"}, nowMs)` +from `lib/tool-cache.mjs` — storing the DIGEST, not the raw rows. + +This is not optional polish; without it the MCP cache goes unused. Before this +was added, the cache went entirely unused across every live run — an agent's +check-then-call-then-store costs three calls on a miss to save one later, so +skipping it is the rational choice for a one-off query. Pre-seeding inverts +that — the agent's `get` is a single call that usually hits. Store the same +digest you put in the evidence file; the two are complementary (the file is +read wholesale at turn 1, the cache answers a specific repeat query later). + +**Also hand every dispatch the tool cache.** The evidence file shares digested +_findings_; `bin/cached-exec.mjs` / `bin/cached-mcp.mjs` share raw _call +results_, which is where most duplicate work actually hides — `gh` calls make +up a large share of all coordinator tool calls on a real build, and a +meaningful number of them are byte-identical commands re-run by different +coordinators. Include the plugin root in each +dispatch prompt so coordinators can invoke the wrappers, and tell them to pass +their own `testRunId` as `writerId`. The cache lives at +`<tmpdir>/bstack-rca/rca-toolcache.<buildId>/`, one file per call key, shared +by shell and MCP alike. Read `node bin/cached-exec.mjs <buildId> --stats` at +the end of the run to report how much it actually saved rather than assuming. + +**Concurrency is handled by layout, not by locking.** Base +(`rca-evidence.<buildId>.json`) has exactly one writer — this orchestrator, in +Step 4. Every coordinator writes only its own shard under +`rca-evidence.<buildId>.contrib/<testRunId>.json`. Since no two processes ever +open the same file for writing, concurrent write-back cannot lose an update; +`readEvidenceFile` folds base + all shards into one view, applying shards in +sorted order, with real evidence taking precedence over a recorded `gap`. A +comparison under a realistic concurrent read→work→write window showed a +single shared file losing the large majority of concurrent updates, while +this sharded layout lost none. + **Application bugs need a culprit PR.** Whenever a test's RCA classifies as PRODUCT_BUG / application bug, the coordinator MUST hunt the culprit PR via the github connector (deploy timeline vs last-pass window, changed paths vs failure -signature — `references/github-evidence.md`) and feed the PR link(s) to TFA in +signature — `<pluginRoot>/skills/rca-build/references/github-evidence.md`) and feed the PR link(s) to TFA in the turn message so the dashboard RCA's `related_prs` populates. An application-bug RCA with no GitHub PR link is **incomplete**: keep digging until the turn cap; if still none, the turn must explicitly state "no culprit PR @@ -202,7 +1097,18 @@ link — that is all. When every row is terminal: (`<N> tests · <R> resolved · <P> pending · <F> failed`). **Nothing per-test.** 2. Call **`triggerRcaReport(buildUuid=<build id>)`** (add `force=true` only to re-run over an existing completed report). -3. Print the link line, verbatim shape: +3. **Only once that call succeeds**, call + `cleanupBuildArtifacts(buildId, config.paths.stateDir)` + (`lib/build-cleanup.mjs`) to delete THIS build's own CSV, evidence file + + `.contrib/` shards, tool cache, and turn1 registry. Never call this before + `triggerRcaReport` succeeds, and never on a run that ends with any row still + non-terminal — at that point resume still needs these files. This is safe + specifically because Step 6 only runs "when every row is terminal": there is + nothing left to resume for THIS build once its report has generated. It is + deliberately not `lib/state-dir.mjs`'s `pruneStateDir` (a separate, manual, + age-based sweep across every build in the shared temp dir) — that remains + the safety net for a build that crashes before ever reaching Step 6. +4. Print the link line, verbatim shape: ``` Full report on the Test Observability UI: <viewReport> @@ -235,7 +1141,12 @@ thread. the gate closes, never ask the user anything.** - An invalid/absent connector is a recorded gap, never a blocker. - Headless + missing build id → end immediately. Headless never asks. -- Never call `tfaRcaTurn` from this skill — always via the `ai-tfa-coordinator`. +- Never call `tfaRcaTurn` from this skill — always via the `ai-tfa-coordinator` — + **except Step 4b's turn-1 pre-dispatch**, which is a deliberate, narrow carve-out + (one direct call per cluster representative, concurrent with Step 4, never a + follow-up turn) documented there. Every OTHER `tfaRcaTurn` call — every turn + past 1, and every sibling's turn 1 — still goes exclusively through a + dispatched coordinator. - A soft-`PENDING` is never an answer: it must be drained with `getTfaTurnResult(testRunId, turnId)` before any further submit on that thread. Only a spent drain budget may end a test `PENDING`. @@ -245,3 +1156,10 @@ thread. the Test Observability UI link only. - A PRODUCT_BUG RCA without a GitHub PR link is incomplete — dig until the turn cap, else state what was searched and record the gap. +- Step 4's first `gh pr list` call per repo MUST include `files` in `--json` — + never split into a plain list followed by a per-PR `gh pr view --json files` + backfill loop. +- Every reference-doc / `lib/` path handed to a coordinator (in the dispatch + prompt or in `agents/ai-tfa-coordinator.md`) MUST be `pluginRoot`-qualified + (`<pluginRoot>/skills/rca-build/references/<file>.md`) — never a bare + `references/<file>.md`, which resolves against an unknown coordinator cwd. diff --git a/skills/rca-build/references/clustering.md b/skills/rca-build/references/clustering.md index 1ac5866..689434d 100644 --- a/skills/rca-build/references/clustering.md +++ b/skills/rca-build/references/clustering.md @@ -1,4 +1,4 @@ -# Failure-signature clustering +# Clustering Why: a red build's N failures usually trace to a handful of causes (one bad PR/deploy/shared helper). Running the full collaborative loop once per *cause* @@ -7,9 +7,33 @@ causes)** — the only thing that makes "RCA for ALL failed tests, even thousand feasible. But **every failed test must still show a per-test RCA in the TRA dashboard**, so clustering collapses the *evidence hunt*, not the *output*. -The logic lives in `lib/signature.mjs`; this file is the protocol. - -## The signature +## Two sources, one contract + +Both paths produce the identical `{ cluster_id, signature, members, +representative, siblings }` shape, so nothing downstream (the fan-out +workflow, the sequential harness) needs to know which one ran: + +- **Preferred — server-computed themes.** `lib/theme-clustering.mjs` → + `clustersFromThemes(rows, themesResult, testsByThemeId)`, fed from the + `getBuildFailureThemes` / `listTestsInFailureTheme` MCP tools (SKILL.md Step + 3). `getBuildFailureThemes` is responsible for making themes exist, not just + reading them — if nothing has ever been computed for this build it triggers + computation (one POST, same call) and polls for `buildThemeWorkflow.status` + to reach `SUCCESS`, up to its own budget. The grouping reflects the + server's own root-cause clustering instead of a text-signature guess — two + failures with an identical error string but unrelated causes are not + conflated the way a client-side "signature" would be. +- **Fallback — client-side failure signature.** `lib/signature.mjs` → + `clusterAndPersist(csvPath, csvStateModule)`, the original text-normalization + approach described below. Used whenever `getBuildFailureThemes` comes back + `ready: false` — still computing past its poll budget, a failure status, or + `status: "trigger-unavailable"` (the trigger call didn't succeed). This + makes the flow independent of whether the trigger call currently succeeds: + whenever it doesn't, every un-computed build degrades straight to this + fallback; whenever it does, the same call reaches `ready: true` on its own + and this fallback simply isn't exercised. + +## The signature (fallback path only) Computed from the trimmed failure detail `listTestIds(includeFailureDetail=true)` already returns on each row — **no extra probe turns**: diff --git a/skills/rca-build/references/evidence-routing.md b/skills/rca-build/references/evidence-routing.md index 05b4435..2c3ba5b 100644 --- a/skills/rca-build/references/evidence-routing.md +++ b/skills/rca-build/references/evidence-routing.md @@ -13,6 +13,13 @@ capability manifest — see `SKILL.md` § Gate Part A). There are **no `kubectl` `chitragupta` / `bifrost` literals here** — that is the whole point of going generic. +**Contents:** [How asks are processed](#how-a-turns-asks-are-processed) · +[Routing table](#routing-table-capability-not-tool) · +[Digest format](#digest-format) · +[Unfulfillable asks](#unfulfillable-asks--report-dont-drop) · +[Capability manifest](#capability-manifest-built-once-at-the-gate) · +[Build-level evidence cache](#build-level-evidence-cache-compute-once) + The registry logic lives in `lib/routing.mjs` (`routeAsk` / `routeAsks`); this file is the human/agent-facing contract for the digest and the size caps. @@ -89,16 +96,21 @@ and unfulfillable variants) — copy it, don't retype it. Shape: | Field / scope | Soft target | Hard ceiling | On exceed | |---|---|---|---| -| `SUMMARY` | ≤ 300 chars | 400 chars | Tighten to the finding; drop restatement of the ask | -| `SNIPPET` per ask | ≤ 20 lines | 40 lines | Keep the load-bearing lines; replace the rest with `… (N lines elided — see LINK)` | -| Code diff in a `product_code` snippet | ≤ 1 hunk | 3 hunks | Show changed lines + 3 lines context; link the full PR | -| Whole next-turn `message` | ≤ 200 lines | 400 lines (and ≤ `turnMessageMaxChars`) | Drop `low`-priority asks first; keep every `high` ask's block | +| `SUMMARY` | ≤ 60 chars | 80 chars | Tighten to the finding; drop restatement of the ask | +| `SNIPPET` per ask | ≤ 4 lines | 8 lines | Keep the load-bearing lines; replace the rest with `… (N lines elided — see LINK)` | +| Code diff in a `product_code` snippet | ≤ 1 hunk | 2 hunks | Show changed lines only, no context lines; link the full PR | +| Whole next-turn `message` | ≤ 40 lines | 80 lines (and ≤ `turnMessageMaxChars`) | Drop `low`-priority asks first; keep every `high` ask's block | | Asks fulfilled per turn | all `high` + `medium` | — | Defer `low` asks to a later turn rather than truncating a `high` ask | Truncation rule of thumb: **never truncate a `high`-priority ask's block to fit a `low`-priority one.** Drop the low block whole; keep the high block intact. The -whole-message ceiling also honors `turnMessageMaxChars` from -`config/rca.config.json` (the tool caps `message` at 5000 chars). +whole-message ceiling honors `turnMessageMaxChars` from +`config/rca.config.json`, now set to **1000 chars** — a plugin-configured +self-limit, tighter than the underlying tool's actual hard cap (the +`tfaRcaTurn` MCP tool itself allows up to 5000 chars per `message`; the plugin +just chooses not to use all of it). At this budget, expect at most 2-3 ask +blocks per turn before hitting the ceiling — defer lower-priority asks to a +follow-up turn rather than cramming everything into one. ### What never goes in a digest diff --git a/skills/rca-build/references/github-evidence.md b/skills/rca-build/references/github-evidence.md index 4b2c445..2960a9c 100644 --- a/skills/rca-build/references/github-evidence.md +++ b/skills/rca-build/references/github-evidence.md @@ -9,6 +9,15 @@ that tries to *disprove* each suspect before it enters `related_prs`. > needed and use whatever the client already has — **GitHub MCP if available, > else `gh`, else degrade** to an `unavailable` block. +**Contents:** [Capability discovery](#capability-discovery-in-order) · +[Culprit-PR hunt](#application-bugs-require-a-culprit-pr-hunt-mandatory) · +[Batching probes](#batch-every-independent-probe-into-one-message--never-one-call-per-turn) · +[Evidence per ask](#evidence-each-ask-needs-be-specific--no-fishing) · +[Field-filtering](#field-filtering--project-before-you-pull-every-call) · +[Falsification protocol](#falsification-protocol--rule-out-dont-just-rule-in) · +[Suspect packet](#the-suspect-packet-structured-not-free-text) · +[Digest discipline](#digest-discipline) + ## Capability discovery (in order) 1. **GitHub MCP** (`mcp__github__*`) — preferred for structured PR/diff/blame queries. @@ -41,6 +50,30 @@ the CSV row records the gap. Never fabricate a PR; if the github connector is invalid/absent, the same explicit statement plus an `unavailable` block goes to TFA (a gate-recorded gap). +## Batch every independent probe into one message — never one call per turn + +This hunt routinely needs several `gh` calls that don't depend on each +other's output: a commit-history check per candidate file in "changed paths +vs failure signature," each row of the "Evidence each ask needs" table +below, and each candidate PR's falsification check. **None of these need to +see a prior result before running** — the only exception is when one call's +output supplies a literal input to the next (e.g., you need a PR number +back from a search before you can `gh pr view` it). + +Issue every independent probe as its own tool call **within the same +message** — the same discipline `ai-tfa-coordinator.md`'s NEEDS_INFO step +already requires across multiple asks (`Promise.all` / concurrent gather) +applies here too, one level down, across multiple probes inside a single +ask. One call per file path, fired one message at a time, waiting for each +result before issuing the next, spends a full turn's think-time on every +individual `gh api` round trip even though the call itself finishes in +under two seconds — for a five-file changed-paths check that is the +difference between one batched message and five serialized ones. Plan the +full probe list first (every candidate file, every table row, every +falsification check that has no dependency on another probe's result), then +fire all of them together; only serialize the ones with a genuine +input-from-output dependency. + ## Evidence each ask needs (be specific — no fishing) | Ask intent | Gather exactly | @@ -55,6 +88,35 @@ Scope everything by the failing test's `file_path` + the error summary. The build-level evidence (diff-since-last-green, PR window) is **pre-computed once** and passed in — reuse it; do not re-fetch per test. +## Field-filtering — project before you pull, every call + +The single most common way a gather call wastes context: pulling a full +object when the ask only needs one or two fields from it. This applies to +whichever connector resolved for `github` (most commonly the `gh` CLI today, +or a GitHub MCP tool) — every call should already be filtered to the field(s) +the ask needs, not filtered after the fact by reading past the noise. The +same discipline applies to `infra` gather calls (`kubectl` or whatever the +manifest resolved to), since the failure mode is identical. + +| Need | Don't — pulls the whole object | Do — projects to the field(s) the ask needs | +|---|---|---| +| Repo exists / default branch | `gh api repos/OWNER/REPO` | `gh api repos/OWNER/REPO --jq '.default_branch'` | +| Branch exists on the shipping branch | `gh api repos/OWNER/REPO/branches/BRANCH` | `gh api repos/OWNER/REPO/branches/BRANCH --jq '.name'` | +| Commit history / PR-window search | `gh api "repos/OWNER/REPO/commits?sha=BRANCH&per_page=100"` | add `--jq '[.[] | {sha: .sha[0:8], date: .commit.committer.date, msg: (.commit.message | split("\n")[0])}]'` | +| PR metadata | `gh pr view N --repo OWNER/REPO` (full payload) | `gh pr view N --repo OWNER/REPO --json state,mergedAt,baseRefName,headRefOid,files,author` — `--json` is itself a field allowlist; list only the fields this ask uses | +| Pod / workload listing | `kubectl get pods -n NS -o wide` | `kubectl get pods -n NS -o custom-columns='NAME:.metadata.name,STATUS:.status.phase'` | +| Deploy / image state | `kubectl get deploy -n NS -o yaml` | `kubectl get deploy -n NS -o custom-columns='NAME:.metadata.name,IMAGE:.spec.template.spec.containers[0].image'` | +| Log sweep | a raw `--tail` dump | `kubectl logs POD --since=<window> --tail=2000 \| grep -E '<correlation token>\|ERROR\|Exception'` — filter by the correlation token, never a raw tail | + +**Never run the unfiltered form "to see the shape first."** An exploratory +raw call costs the same context whether or not its output ends up in the +digest — a bare repo or commit object routinely carries license/URL metadata +and a multi-hundred-character signature block that no evidence ask ever +consults. If the exact field path is genuinely unknown, learn the shape from +one throwaway call against a cheap target, then filter every real call from +that point on — never repeat the unfiltered form per repo, per PR, or per +test. + ## Falsification protocol — rule out, don't just rule in For **each** candidate suspect PR, try to **break** the hypothesis: diff --git a/skills/rca-build/templates/evidence-block.md b/skills/rca-build/templates/evidence-block.md index fa0bf49..97b41f7 100644 --- a/skills/rca-build/templates/evidence-block.md +++ b/skills/rca-build/templates/evidence-block.md @@ -6,12 +6,12 @@ the forbidden list: `../references/evidence-routing.md`. Fulfilled ask: ``` -ASK: <verbatim `what` from the TfaAsk, ≤ 120 chars> +ASK: <verbatim `what` from the TfaAsk, ≤ 80 chars> TYPE: <evidenceType> FOUND: <yes | no | partial> -SUMMARY: <1–3 sentences — the finding, in the agent's words. ≤ 400 chars> +SUMMARY: <1 sentence — the finding, in the agent's words. ≤ 80 chars> SNIPPET: - <the load-bearing excerpt only — see size caps. Omit if a LINK fully carries it.> + <the load-bearing excerpt only, ≤ 4 lines — see size caps. Omit if a LINK fully carries it.> LINK: <permalink to the source — PR/commit/log-search/metrics panel/deploy record. Omit if N/A.> ``` diff --git a/tests/build-cleanup.test.mjs b/tests/build-cleanup.test.mjs new file mode 100644 index 0000000..e903052 --- /dev/null +++ b/tests/build-cleanup.test.mjs @@ -0,0 +1,88 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, existsSync, mkdirSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { cleanupBuildArtifacts } from "../lib/build-cleanup.mjs"; +import { csvPathFor } from "../lib/csv-state.mjs"; +import { evidencePathFor, contribDirFor } from "../lib/evidence-file.mjs"; +import { toolCacheDirFor } from "../lib/tool-cache.mjs"; +import { turn1PathFor } from "../lib/turn1-registry.mjs"; + +function fixture() { + return mkdtempSync(join(tmpdir(), "rca-cleanup-")); +} + +// Writes every artifact family for one build, so a test can assert cleanup +// removed exactly (and only) what that build produced. +function seedBuild(buildId, dir) { + writeFileSync(csvPathFor(buildId, dir), "buildId,testRunId\n"); + writeFileSync(evidencePathFor(buildId, dir), "{}"); + const contrib = contribDirFor(evidencePathFor(buildId, dir)); + mkdirSync(contrib, { recursive: true }); + writeFileSync(join(contrib, "3900000001.json"), "{}"); + const cache = toolCacheDirFor(buildId, dir); + mkdirSync(cache, { recursive: true }); + writeFileSync(join(cache, "entry.json"), "{}"); + writeFileSync(turn1PathFor(buildId, dir), "{}"); +} + +test("cleanupBuildArtifacts deletes every artifact family for the given build", () => { + const dir = fixture(); + seedBuild("b1", dir); + + const r = cleanupBuildArtifacts("b1", dir); + + assert.equal(existsSync(csvPathFor("b1", dir)), false); + assert.equal(existsSync(evidencePathFor("b1", dir)), false); + assert.equal(existsSync(contribDirFor(evidencePathFor("b1", dir))), false); + assert.equal(existsSync(toolCacheDirFor("b1", dir)), false); + assert.equal(existsSync(turn1PathFor("b1", dir)), false); + assert.equal(r.deleted.length, 5); + assert.deepEqual(r.errors, []); + + rmSync(dir, { recursive: true, force: true }); +}); + +// The load-bearing test: a concurrent run over a DIFFERENT build in the same +// stateDir must survive this build's cleanup untouched — nothing here should +// ever glob/sweep the shared directory. +test("cleanupBuildArtifacts never touches a different build's artifacts in the same stateDir", () => { + const dir = fixture(); + seedBuild("b1", dir); + seedBuild("b2", dir); + + cleanupBuildArtifacts("b1", dir); + + assert.equal(existsSync(csvPathFor("b1", dir)), false); + assert.equal(existsSync(csvPathFor("b2", dir)), true, "other build's CSV must survive"); + assert.equal(existsSync(evidencePathFor("b2", dir)), true, "other build's evidence file must survive"); + assert.equal(existsSync(contribDirFor(evidencePathFor("b2", dir))), true, "other build's shards must survive"); + assert.equal(existsSync(toolCacheDirFor("b2", dir)), true, "other build's tool cache must survive"); + assert.equal(existsSync(turn1PathFor("b2", dir)), true, "other build's turn1 registry must survive"); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("cleanupBuildArtifacts is a no-op, not a throw, when nothing was ever written for this build", () => { + const dir = fixture(); + const r = cleanupBuildArtifacts("never-ran", dir); + assert.deepEqual(r, { deleted: [], errors: [] }); + rmSync(dir, { recursive: true, force: true }); +}); + +test("cleanupBuildArtifacts tolerates a build with only some artifact families present", () => { + const dir = fixture(); + // Only the CSV and turn1 registry exist — no evidence file, no tool cache + // (e.g. an unclustered rerun that never hit Step 4b's NEEDS_INFO/PENDING path). + writeFileSync(csvPathFor("b1", dir), "buildId,testRunId\n"); + writeFileSync(turn1PathFor("b1", dir), "{}"); + + const r = cleanupBuildArtifacts("b1", dir); + + assert.equal(r.deleted.length, 2); + assert.equal(existsSync(csvPathFor("b1", dir)), false); + assert.equal(existsSync(turn1PathFor("b1", dir)), false); + + rmSync(dir, { recursive: true, force: true }); +}); diff --git a/tests/conformance.test.mjs b/tests/conformance.test.mjs index 33ccbbb..d9af539 100644 --- a/tests/conformance.test.mjs +++ b/tests/conformance.test.mjs @@ -174,6 +174,67 @@ test("a failed read is not a verdict — the drain keeps reading and still lands assert.equal(reads, 2); }); +test("a PERSISTENT hard error stops the drain early instead of burning the budget", async () => { + const fx = load("soft-pending-drain.json"); + let reads = 0; + const result = await runRcaLoop({ + testRunId: fx.testRunId, + submit: replaySubmit([fx.turns[0]]), // always soft-PENDING + readTurn: async () => { + reads++; + throw new Error("Failed to get tfa turn result: TFA agent run failed"); + }, + // Budget allows 40 reads; the error cap must cut it off long before that. + config: { ...CONFIG, softPendingDrain: { maxWaitMs: 600_000, intervalMs: 1, maxReads: 40, maxErrorReads: 3 } }, + sleep: noSleep, + }); + assert.equal(result.status, "PENDING"); + assert.equal(reads, 3, "stopped at maxErrorReads, not the 40-read budget"); + assert.match(result.root_cause, /tfa-error/); + assert.equal(result.turnId, "c2e1a6fd-2243-4f93-bc69-62f298db062c"); // still resumable +}); + +test("an error-shaped RESULT (not thrown) also trips the fast-fail", async () => { + const fx = load("soft-pending-drain.json"); + let reads = 0; + const result = await runRcaLoop({ + testRunId: fx.testRunId, + submit: replaySubmit([fx.turns[0]]), + // The MCP tool reports the wedge as a returned payload, not an exception. + readTurn: async () => { + reads++; + return { status: "ERROR", message: "TFA agent run failed" }; + }, + config: { ...CONFIG, softPendingDrain: { maxWaitMs: 600_000, intervalMs: 1, maxReads: 40, maxErrorReads: 2 } }, + sleep: noSleep, + }); + assert.equal(result.status, "PENDING"); + assert.equal(reads, 2); + assert.match(result.root_cause, /tfa-error/); +}); + +test("INTERMITTENT errors do not trip the fast-fail — a good read clears the streak", async () => { + const fx = load("soft-pending-drain.json"); + const landed = fx.reads[2]; + let reads = 0; + const result = await runRcaLoop({ + testRunId: fx.testRunId, + submit: replaySubmit(fx.turns), + readTurn: async () => { + reads++; + // fail, ok, fail, ok, ... never 2 consecutive failures + if (reads % 2 === 1) throw new Error("transient 502"); + return reads < 6 ? { status: "PENDING" } : landed; + }, + config: { ...CONFIG, softPendingDrain: { maxWaitMs: 600_000, intervalMs: 1, maxReads: 40, maxErrorReads: 2 } }, + manifest: GITHUB_AVAILABLE, + gather, + sleep: noSleep, + }); + assert.equal(result.status, "RESOLVED", "flaky-but-recovering reads must still land"); + assert.equal(reads, 6); +}); + test("BLOCKED surfaced by a drain is terminal — no empty resubmits to the turn cap", async () => { const fx = load("soft-pending-drain.json"); let submits = 0; diff --git a/tests/csv-state.test.mjs b/tests/csv-state.test.mjs index 3cbc83a..6070ef4 100644 --- a/tests/csv-state.test.mjs +++ b/tests/csv-state.test.mjs @@ -1,12 +1,13 @@ import { test, beforeEach, afterEach } from "node:test"; import assert from "node:assert/strict"; -import { mkdtempSync, rmSync } from "node:fs"; +import { mkdtempSync, rmSync, writeFileSync, mkdirSync, chmodSync, statSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { csvPathFor, seed, readRows, + writeRows, claim, heartbeat, flip, @@ -121,6 +122,28 @@ test("pendingRows returns only pending work", () => { assert.equal(pend[0].testRunId, "102"); }); +// Regression: `flip` used to accept ONLY the lowercase CSV vocabulary and +// return a bare `false` for anything else — including `RESOLVED`, the exact +// value the RCA_OUTPUT contract mandates. A whole batch of coordinator results +// was lost that way: they called flip, got a silent no-op, and the rows stayed +// `pending` looking un-run. +test("flip accepts the RCA_OUTPUT vocabulary and normalizes it", () => { + seed(csv, "build-1", TESTS); + assert.equal(flip(csv, 101, { rca_done: "RESOLVED", root_cause: "x" }, 1000), true); + assert.equal(readRows(csv).find((r) => r.testRunId === "101").rca_done, "resolved"); + + assert.equal(flip(csv, 102, { status: "PENDING" }, 1000), true); + assert.equal(readRows(csv).find((r) => r.testRunId === "102").rca_done, "pending-resume"); +}); + +test("flip maps the output block's field names onto real columns", () => { + seed(csv, "build-1", TESTS); + flip(csv, 101, { rca_done: "resolved", thread_id: "chat:101", turn_id: "t-7" }, 1000); + const row = readRows(csv).find((r) => r.testRunId === "101"); + assert.equal(row.threadId, "chat:101"); + assert.equal(row.turnId, "t-7"); +}); + test("flip rejects a missing/non-terminal rca_done without mutating the row", () => { seed(csv, "build-1", TESTS); claim(csv, 101, "w1", 1000); @@ -185,3 +208,108 @@ test("csvPathFor: stateDir override wins over temp", () => { const p = csvPathFor("b1", "/ci/artifacts"); assert.equal(p, join("/ci/artifacts", "rca-state.b1.csv")); }); + +// A foreign header must fail loudly, because writeRows only emits COLUMNS and +// would silently drop anything it didn't recognise. A real legacy 10-column +// file lost test_id and test_name this way while reporting success. +test("readRows refuses a foreign schema instead of silently dropping columns", () => { + const dir = mkdtempSync(join(tmpdir(), "rca-legacy-")); + const p = join(dir, "legacy.csv"); + writeFileSync(p, "test_id,test_name,rca_done\nt1,login spec,pending\n", "utf8"); + + assert.throws(() => readRows(p), /unrecognised column/i, + "must name the problem rather than mangle the file"); + assert.throws(() => readRows(p), /test_id/, "must say WHICH columns"); + + rmSync(dir, { recursive: true, force: true }); +}); + +// Known legacy spellings are still accepted — the guard is for genuinely +// foreign schemas, not for every older name. +test("readRows maps aliased header names rather than rejecting them", () => { + const dir = mkdtempSync(join(tmpdir(), "rca-alias-")); + const p = join(dir, "aliased.csv"); + writeFileSync(p, "test_run_id,status,thread_id\n42,pending,th-1\n", "utf8"); + + const rows = readRows(p); + assert.equal(rows[0].testRunId, "42"); + assert.equal(rows[0].rca_done, "pending"); + assert.equal(rows[0].threadId, "th-1"); + + rmSync(dir, { recursive: true, force: true }); +}); + +// mkdirSync's `mode` applies on CREATE only, so a directory made before the +// hardening landed keeps 0755 forever — with root causes and culprit PRs in it. +test("writeRows tightens a pre-existing world-readable state dir", () => { + const dir = mkdtempSync(join(tmpdir(), "rca-perm-")); + const loose = join(dir, "loose"); + mkdirSync(loose, { mode: 0o755 }); + chmodSync(loose, 0o755); // as an older version would have left it + + const csv = join(loose, "rca-state.b.csv"); + writeRows(csv, []); + + assert.equal(statSync(loose).mode & 0o777, 0o700, "existing dir must be tightened, not left open"); + assert.equal(statSync(csv).mode & 0o777, 0o600); + + rmSync(dir, { recursive: true, force: true }); +}); + +// turnId only exists on a soft-PENDING turn, which is exactly the case that +// produces pending-resume. Without it the resume path submits blind onto a +// thread that still has a turn in flight — and the row looks healthy in the CSV. +test("flipping to pending-resume without a turnId warns loudly", () => { + const dir = mkdtempSync(join(tmpdir(), "rca-resume-")); + const csv = join(dir, "s.csv"); + seed(csv, "b", [{ test_id: 1, test_name: "t" }, { test_id: 2, test_name: "u" }]); + + const warnings = []; + const orig = console.warn; + console.warn = (m) => warnings.push(String(m)); + try { + flip(csv, 1, { rca_done: "pending-resume" }, 1000); + flip(csv, 2, { rca_done: "pending-resume", turnId: "abc-123" }, 1000); + } finally { + console.warn = orig; + } + + const noTurn = warnings.filter((w) => /NO turnId/.test(w)); + assert.equal(noTurn.length, 1, "exactly the seedless row must warn"); + assert.match(noTurn[0], /submit blind/); + assert.equal(readRows(csv).find((r) => r.testRunId === "2").turnId, "abc-123"); + + // Still resumable either way — warning, not rejection. + assert.equal(readRows(csv).find((r) => r.testRunId === "1").rca_done, "pending-resume"); + + rmSync(dir, { recursive: true, force: true }); +}); + +// "A PRODUCT_BUG RCA without a culprit PR is incomplete" was a prompt-only rule. +// A stated "none — searched X" satisfies it; a blank field does not, and the two +// are indistinguishable in the CSV. +test("flip warns on a product bug with no PR evidence trail", () => { + const dir = mkdtempSync(join(tmpdir(), "rca-pb-")); + const csv = join(dir, "s.csv"); + seed(csv, "b", [{ test_id: 1 }, { test_id: 2 }, { test_id: 3 }]); + + const warnings = []; + const orig = console.warn; + console.warn = (m) => warnings.push(String(m)); + try { + flip(csv, 1, { rca_done: "resolved", failure_type: "PRODUCT_BUG" }, 1); + flip(csv, 2, { rca_done: "resolved", failure_type: "PRODUCT_BUG", related_prs: ["https://x/pull/1"] }, 1); + flip(csv, 3, { rca_done: "resolved", failure_type: "PRODUCT_BUG", related_prs: "none — searched repo-a, repo-b in window" }, 1); + } finally { + console.warn = orig; + } + + const pb = warnings.filter((w) => /EMPTY related_prs/.test(w)); + assert.equal(pb.length, 1, "only the blank one warns"); + assert.match(pb[0], /testRunId=1/); + + // An honest dead end is compliant — must not be nagged. + assert.ok(!pb.some((w) => /testRunId=3/.test(w)), "a stated 'none, searched X' satisfies the rule"); + + rmSync(dir, { recursive: true, force: true }); +}); diff --git a/tests/evidence-file.test.mjs b/tests/evidence-file.test.mjs new file mode 100644 index 0000000..b9a84d8 --- /dev/null +++ b/tests/evidence-file.test.mjs @@ -0,0 +1,450 @@ +import { test, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, writeFileSync, statSync, chmodSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + evidencePathFor, + emptyEvidenceFile, + initEvidenceFile, + readEvidenceFile, + writeEvidenceFile, + setBaseline, + setGithubEvidence, + setLogsEvidence, + contributeGithubEvidence, + contributeLogsEvidence, + contribDirFor, + contribPathFor, + readBaseFile, + hasTrustworthyPrList, + recomputeCoverage, +} from "../lib/evidence-file.mjs"; + +let dir; +let file; + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "rca-evidence-")); + file = join(dir, "evidence.json"); +}); +afterEach(() => rmSync(dir, { recursive: true, force: true })); + +test("evidencePathFor: build id is in the filename, default dir is OS temp", () => { + const p = evidencePathFor("abc123XYZ"); + assert.ok(p.startsWith(join(tmpdir(), "bstack-rca"))); + assert.ok(p.endsWith("rca-evidence.abc123XYZ.json")); +}); + +test("evidencePathFor: different builds never share a path", () => { + assert.notEqual(evidencePathFor("build-A"), evidencePathFor("build-B")); +}); + +test("evidencePathFor: sanitizes hostile ids and handles empty", () => { + assert.ok(evidencePathFor("../../etc/passwd").endsWith("rca-evidence..._.._etc_passwd.json")); + assert.ok(evidencePathFor("").endsWith("rca-evidence.unknown-build.json")); +}); + +test("evidencePathFor: stateDir override wins over temp", () => { + const p = evidencePathFor("b1", "/ci/artifacts"); + assert.equal(p, join("/ci/artifacts", "rca-evidence.b1.json")); +}); + +test("readEvidenceFile on a missing path returns the empty shape, never throws", () => { + const doc = readEvidenceFile(file); + assert.deepEqual(doc, emptyEvidenceFile("unknown-build", 0)); +}); + +test("initEvidenceFile creates the file with the given buildId", () => { + const doc = initEvidenceFile(file, "build-1", 1000); + assert.equal(doc.buildId, "build-1"); + assert.equal(doc.generatedAtMs, 1000); + assert.deepEqual(readEvidenceFile(file), doc); +}); + +test("initEvidenceFile is idempotent — does not clobber an existing file", () => { + initEvidenceFile(file, "build-1", 1000); + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "x" } }, 2000); + const before = readEvidenceFile(file); + const again = initEvidenceFile(file, "build-1", 9999); + assert.deepEqual(again, before); +}); + +test("setGithubEvidence and setLogsEvidence coexist without clobbering each other", () => { + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "a-deploy" } }, 1000); + setLogsEvidence(file, "workload-1", { gap: null, kubectlSweep: { block: "w1-logs" } }, 1000); + const doc = readEvidenceFile(file); + assert.equal(doc.github["org/a"].deployState.block, "a-deploy"); + assert.equal(doc.logs["workload-1"].kubectlSweep.block, "w1-logs"); +}); + +test("setGithubEvidence for a second repo does not disturb the first", () => { + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "a" } }, 1000); + setGithubEvidence(file, "org/b", { gap: null, deployState: { block: "b" } }, 1000); + const doc = readEvidenceFile(file); + assert.equal(doc.github["org/a"].deployState.block, "a"); + assert.equal(doc.github["org/b"].deployState.block, "b"); +}); + +test("setGithubEvidence twice for the SAME repo overwrites only that repo", () => { + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "old" } }, 1000); + setGithubEvidence(file, "org/b", { gap: null, deployState: { block: "b" } }, 1000); + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "new" } }, 2000); + const doc = readEvidenceFile(file); + assert.equal(doc.github["org/a"].deployState.block, "new"); + assert.equal(doc.github["org/b"].deployState.block, "b"); // untouched +}); + +test("setBaseline records baseline and suspectWindow without touching github/logs", () => { + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "a" } }, 1000); + setBaseline(file, { ref: "sha123", isFallback: false }, { reposRequested: ["org/a"] }, 2000); + const doc = readEvidenceFile(file); + assert.deepEqual(doc.baseline, { ref: "sha123", isFallback: false }); + assert.deepEqual(doc.suspectWindow, { reposRequested: ["org/a"] }); + assert.equal(doc.github["org/a"].deployState.block, "a"); // untouched +}); + +test("recomputeCoverage: a covered repo/workload has no gap; a missing one is gapped", () => { + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "a" } }, 1000); + setLogsEvidence(file, "w1", { gap: null, kubectlSweep: { block: "w1" } }, 1000); + const coverage = recomputeCoverage( + file, + { repos: ["org/a", "org/b"], workloads: ["w1", "w2"] }, + 2000, + ); + assert.deepEqual(coverage.reposCovered, ["org/a"]); + assert.deepEqual(coverage.reposGapped, ["org/b"]); + assert.deepEqual(coverage.workloadsCovered, ["w1"]); + assert.deepEqual(coverage.workloadsGapped, ["w2"]); +}); + +test("recomputeCoverage: a present entry with a non-null gap is NOT covered", () => { + setGithubEvidence(file, "org/a", { gap: "gh auth failed for this repo" }, 1000); + const coverage = recomputeCoverage(file, { repos: ["org/a"], workloads: [] }, 2000); + assert.deepEqual(coverage.reposCovered, []); + assert.deepEqual(coverage.reposGapped, ["org/a"]); +}); + +test("recomputeCoverage persists onto the file (readable afterwards)", () => { + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "a" } }, 1000); + recomputeCoverage(file, { repos: ["org/a"], workloads: [] }, 2000); + const doc = readEvidenceFile(file); + assert.deepEqual(doc.coverage.reposCovered, ["org/a"]); +}); + +test("a block string with newlines and quotes round-trips through JSON unchanged", () => { + const block = 'ASK: did X change?\nTYPE: product_code\nFOUND: yes\nSUMMARY: "quoted" finding\nSNIPPET: line1\nline2'; + setGithubEvidence(file, "org/a", { gap: null, deployState: { block } }, 1000); + const doc = readEvidenceFile(file); + assert.equal(doc.github["org/a"].deployState.block, block); +}); + +test("contribute writes a shard, never the base file", () => { + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "base" } }, 1000); + contributeGithubEvidence(file, "3895581484", "org/a", { + deployState: { block: "coordinator's full diff" }, + }, 2000); + // base is untouched... + assert.equal(readBaseFile(file).github["org/a"].deployState.block, "base"); + // ...but the folded view shows the contribution + assert.equal(readEvidenceFile(file).github["org/a"].deployState.block, "coordinator's full diff"); +}); + +test("contribPathFor: one file per writer, under the build's contrib dir", () => { + const p = contribPathFor(file, "3895581484"); + assert.ok(p.startsWith(contribDirFor(file))); + assert.ok(p.endsWith("3895581484.json")); + assert.notEqual(contribPathFor(file, "w1"), contribPathFor(file, "w2")); +}); + +test("contribPathFor sanitizes a hostile writerId", () => { + assert.ok(contribPathFor(file, "../../etc/passwd").endsWith("_.._etc_passwd.json")); +}); + +test("CONCURRENCY: two writers on the same repo both survive (no lost update)", () => { + setGithubEvidence(file, "org/a", { + gap: null, deployState: { block: "base" }, prsInWindow: [{ pr: "#1" }], + }, 1000); + // Interleave the two writers the way real concurrent coordinators would: + // each reads, then each writes — under a single shared file this is exactly + // the sequence that drops the first writer's update. + contributeGithubEvidence(file, "writerA", "org/a", { prsInWindow: [{ pr: "#2", by: "A" }] }, 2000); + contributeGithubEvidence(file, "writerB", "org/a", { prsInWindow: [{ pr: "#3", by: "B" }] }, 2000); + const prs = readEvidenceFile(file).github["org/a"].prsInWindow.map((p) => p.pr).sort(); + assert.deepEqual(prs, ["#1", "#2", "#3"]); // base + BOTH contributions +}); + +test("CONCURRENCY: two writers on the same workload both survive", () => { + contributeLogsEvidence(file, "writerA", "w1", { kubectlSweep: { block: "A found 3 lines" } }, 1000); + contributeLogsEvidence(file, "writerB", "w1", { victorialogs: { block: "B found 5xx" } }, 1000); + const w = readEvidenceFile(file).logs["w1"]; + assert.equal(w.kubectlSweep.block, "A found 3 lines"); + assert.equal(w.victorialogs.block, "B found 5xx"); +}); + +test("fold: real contributed evidence beats a base-recorded gap", () => { + setGithubEvidence(file, "org/a", { gap: "gh auth failed" }, 1000); + contributeGithubEvidence(file, "w1", "org/a", { + gap: null, deployState: { block: "reachable after all" }, + }, 2000); + const entry = readEvidenceFile(file).github["org/a"]; + assert.equal(entry.gap, null); + assert.equal(entry.deployState.block, "reachable after all"); +}); + +test("fold: a contributed gap does NOT overwrite real base evidence", () => { + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "real base evidence" } }, 1000); + contributeGithubEvidence(file, "w1", "org/a", { deployState: { gap: "my call failed" } }, 2000); + assert.equal(readEvidenceFile(file).github["org/a"].deployState.block, "real base evidence"); +}); + +test("fold: same PR number contributed later wins (deeper finding replaces placeholder)", () => { + setGithubEvidence(file, "org/a", { + gap: null, prsInWindow: [{ pr: "#9011", verdict: "unassessed", files: null }], + }, 1000); + contributeGithubEvidence(file, "w1", "org/a", { + prsInWindow: [{ pr: "#9011", verdict: "supported", files: ["Foo.java"] }], + }, 2000); + const prs = readEvidenceFile(file).github["org/a"].prsInWindow; + assert.equal(prs.length, 1); + assert.equal(prs[0].verdict, "supported"); +}); + +test("fold: contributing a repo the pre-fetch never named", () => { + contributeGithubEvidence(file, "w1", "org/brand-new", { + prsInWindow: [{ pr: "#8912", verdict: "supported" }], + }, 1000); + assert.equal(readEvidenceFile(file).github["org/brand-new"].prsInWindow[0].pr, "#8912"); +}); + +test("fold: clusterIds union across base and multiple shards", () => { + setLogsEvidence(file, "w1", { gap: null, clusterIds: ["c-A"], kubectlSweep: { block: "x" } }, 1000); + contributeLogsEvidence(file, "w1writer", "w1", { clusterIds: ["c-B"] }, 2000); + contributeLogsEvidence(file, "w2writer", "w1", { clusterIds: ["c-C"] }, 2000); + assert.deepEqual(readEvidenceFile(file).logs["w1"].clusterIds.sort(), ["c-A", "c-B", "c-C"]); +}); + +test("fold: a corrupt shard is skipped, not fatal", () => { + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "base" } }, 1000); + contributeGithubEvidence(file, "good", "org/a", { prsInWindow: [{ pr: "#2" }] }, 2000); + writeFileSync(contribPathFor(file, "corrupt"), "{not json", "utf8"); + const doc = readEvidenceFile(file); // must not throw + assert.equal(doc.github["org/a"].prsInWindow[0].pr, "#2"); +}); + +test("recomputeCoverage counts a coordinator-filled gap as covered", () => { + setGithubEvidence(file, "org/a", { gap: "unreachable at pre-fetch time" }, 1000); + let cov = recomputeCoverage(file, { repos: ["org/a"], workloads: [] }, 2000); + assert.deepEqual(cov.reposGapped, ["org/a"]); + contributeGithubEvidence(file, "w1", "org/a", { gap: null, deployState: { block: "got it" } }, 3000); + cov = recomputeCoverage(file, { repos: ["org/a"], workloads: [] }, 4000); + assert.deepEqual(cov.reposCovered, ["org/a"]); + assert.deepEqual(cov.reposGapped, []); +}); + +// Regression: an empty prsInWindow with gap:null used to read as "searched, +// found none" when it may simply never have been populated. Observed live — +// a file asserted 0 PRs for a repo that actually had 21, which would have let +// a coordinator conclude "no culprit PR" with false confidence. +test("empty prsInWindow is NOT coverage unless the search is recorded", () => { + setGithubEvidence(file, "org/never-searched", { gap: null, deployState: { block: "d" }, prsInWindow: [] }, 1000); + setGithubEvidence(file, "org/searched-empty", { gap: null, deployState: { block: "d" }, prsInWindow: [], prsSearched: true }, 1000); + const cov = recomputeCoverage(file, { repos: ["org/never-searched", "org/searched-empty"], workloads: [] }, 2000); + // Both repos ARE covered (each has deploy state) — but only one has a PR + // list safe to read as "no PRs in window". + assert.deepEqual(cov.reposCovered.sort(), ["org/never-searched", "org/searched-empty"]); + assert.deepEqual(cov.reposWithUntrustedPrList, ["org/never-searched"]); +}); + +test("hasTrustworthyPrList distinguishes searched-empty from never-populated", () => { + setGithubEvidence(file, "org/a", { gap: null, prsInWindow: [] }, 1000); + setGithubEvidence(file, "org/b", { gap: null, prsInWindow: [], prsSearched: true }, 1000); + setGithubEvidence(file, "org/c", { gap: null, prsInWindow: [{ pr: "#1" }] }, 1000); + const doc = readEvidenceFile(file); + assert.equal(hasTrustworthyPrList(doc, "org/a"), false); + assert.equal(hasTrustworthyPrList(doc, "org/b"), true); + assert.equal(hasTrustworthyPrList(doc, "org/c"), true); +}); + +test("contributing a PR list records that the search actually ran", () => { + contributeGithubEvidence(file, "w1", "org/a", { prsInWindow: [] }, 1000); + assert.equal(hasTrustworthyPrList(readEvidenceFile(file), "org/a"), true); +}); + +test("prsSearched is sticky — a later non-searching contributor cannot downgrade it", () => { + setGithubEvidence(file, "org/a", { gap: null, prsInWindow: [{ pr: "#1" }], prsSearched: true }, 1000); + contributeGithubEvidence(file, "w1", "org/a", { deployState: { block: "just deploy info" } }, 2000); + assert.equal(readEvidenceFile(file).github["org/a"].prsSearched, true); +}); + +test("a pre-existing loose-mode file is tightened to 0600 on the next write", () => { + writeEvidenceFile(file, emptyEvidenceFile("b", 0)); + chmodSync(file, 0o644); // simulate a file left by a pre-hardening run + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "x" } }, 1000); + assert.equal(statSync(file).mode & 0o777, 0o600); +}); + +test("evidence file and contribution shards are owner-only (0600)", () => { + setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "private PR detail" } }, 1000); + contributeGithubEvidence(file, "w1", "org/a", { prsInWindow: [{ pr: "#1" }] }, 2000); + assert.equal(statSync(file).mode & 0o777, 0o600); + assert.equal(statSync(contribPathFor(file, "w1")).mode & 0o777, 0o600); +}); + +test("writeEvidenceFile creates the parent directory if missing", () => { + const nested = join(dir, "nested", "sub", "evidence.json"); + writeEvidenceFile(nested, emptyEvidenceFile("build-1", 0)); + assert.deepEqual(readEvidenceFile(nested).buildId, "build-1"); +}); + +// Staleness: the resume-path analogue of refusing a branch name. +test("stalenessOf flags an old pre-fetch but never invalidates it", async () => { + const dir = mkdtempSync(join(tmpdir(), "rca-stale-")); + const { evidencePathFor, initEvidenceFile, stalenessOf, readEvidenceFile } = + await import("../lib/evidence-file.mjs"); + const t0 = 1_700_000_000_000; + const p = evidencePathFor("b-stale", dir); + initEvidenceFile(p, "b-stale", t0); + + const fresh = stalenessOf(p, t0 + 5 * 60 * 1000); + assert.equal(fresh.stale, false, "5m into a run is fresh"); + assert.equal(fresh.known, true); + + const old = stalenessOf(p, t0 + 20 * 60 * 60 * 1000); + assert.equal(old.stale, true, "an overnight resume must be flagged"); + assert.match(old.note, /re-verify/, "must say what to do, not just that it is old"); + + // Crucially it is a SIGNAL, not an expiry — the data is still there, because + // stale build-level context still beats none and the failure window is fixed. + assert.ok(readEvidenceFile(p), "file must remain readable when stale"); + + rmSync(dir, { recursive: true, force: true }); +}); + +// The clamp-to-zero trap: a future timestamp must not read as "fresh". +test("stalenessOf refuses to call a future timestamp fresh", async () => { + const dir = mkdtempSync(join(tmpdir(), "rca-skew-")); + const { evidencePathFor, initEvidenceFile, stalenessOf } = await import("../lib/evidence-file.mjs"); + const t0 = 1_700_000_000_000; + const p = evidencePathFor("b-skew", dir); + initEvidenceFile(p, "b-skew", t0); + + // Coordinator's clock is behind the gate's, or the stamp was seeded by hand. + const s = stalenessOf(p, t0 - 11 * 60 * 60 * 1000); + assert.equal(s.stale, true, "unknown age must fail closed, not report fresh"); + assert.equal(s.known, false, "we genuinely cannot compute an age here"); + assert.match(s.note, /future/); + + rmSync(dir, { recursive: true, force: true }); +}); + +// The sha lived only in prose, so the one consumer that needs it structurally +// got an empty map — silently downgrading every local read to a network call. +test("deployShas prefers the explicit field and falls back to the summary", async () => { + const dir = mkdtempSync(join(tmpdir(), "rca-pins-")); + const { evidencePathFor, initEvidenceFile, setGithubEvidence, deployShas } = + await import("../lib/evidence-file.mjs"); + const p = evidencePathFor("b-pins", dir); + initEvidenceFile(p, "b-pins", 1); + + setGithubEvidence(p, "org/explicit", { deployState: { sha: "abc1234", summary: "" } }, 2); + setGithubEvidence(p, "org/prose", { + deployState: { summary: "Branch tip on main at build start = cd88535b (deploy proxy). Redeploy stamped 260731135020Z." }, + }, 3); + setGithubEvidence(p, "org/none", { deployState: { summary: "no sha here" } }, 4); + + const { pins, source } = deployShas(p); + assert.equal(pins["org/explicit"], "abc1234"); + assert.equal(source["org/explicit"], "field"); + assert.equal(pins["org/prose"], "cd88535b", "must recover the sha from prose"); + assert.equal(source["org/prose"], "parsed-from-summary"); + assert.equal(pins["org/none"], undefined, "absent must stay absent, not guess"); + + // The timestamp 260731135020Z is hex-ish and long — anchoring on the + // build-start phrase is what stops it being mistaken for a commit. + assert.notEqual(pins["org/prose"], "260731135020"); + + rmSync(dir, { recursive: true, force: true }); +}); + +// Observed live: a coordinator wrote back a 6-PR window and the file kept ONE, +// with `pr: undefined`, while still flagging the search trustworthy. Cause: +// String(undefined) is the constant "undefined", so every numberless PR +// collided on a single dedupe key. +test("numberless PRs do not collapse into one another", async () => { + const dir = mkdtempSync(join(tmpdir(), "rca-prkey-")); + const { evidencePathFor, initEvidenceFile, contributeGithubEvidence, readEvidenceFile } = + await import("../lib/evidence-file.mjs"); + const p = evidencePathFor("b-prkey", dir); + initEvidenceFile(p, "b-prkey", 1); + + contributeGithubEvidence(p, "w1", "org/r", { + prsSearched: true, + prsInWindow: [{ title: "first" }, { title: "second" }, { title: "third" }], + }, 2); + const got = readEvidenceFile(p).github["org/r"].prsInWindow; + assert.equal(got.length, 3, "three distinct unnumbered PRs must all survive"); + assert.deepEqual(got.map((x) => x.title), ["first", "second", "third"]); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("numbered PRs still merge across writers, string or numeric", async () => { + const dir = mkdtempSync(join(tmpdir(), "rca-prnum-")); + const { evidencePathFor, initEvidenceFile, contributeGithubEvidence, readEvidenceFile } = + await import("../lib/evidence-file.mjs"); + const p = evidencePathFor("b-prnum", dir); + initEvidenceFile(p, "b-prnum", 1); + + contributeGithubEvidence(p, "w1", "org/r", { prsInWindow: [{ pr: "#10", title: "a" }] }, 2); + contributeGithubEvidence(p, "w2", "org/r", { prsInWindow: [{ pr: 10, title: "a-updated" }] }, 3); + + const got = readEvidenceFile(p).github["org/r"].prsInWindow; + assert.equal(got.length, 1, "'#10' and 10 are the same PR"); + assert.equal(got[0].title, "a-updated", "later writer wins"); + + rmSync(dir, { recursive: true, force: true }); +}); + +// Prompting agents to use evidence-show wasn't enough: 21 of 25 reads on a real +// run were raw cat/grep/Read against the base path, each silently missing every +// contribution shard. The file now announces that in its own first bytes. +test("a raw read of the base file announces that it is partial", async () => { + const dir = mkdtempSync(join(tmpdir(), "rca-warn-")); + const { evidencePathFor, initEvidenceFile, setGithubEvidence, readEvidenceFile, readBaseFile } = + await import("../lib/evidence-file.mjs"); + const { readFileSync } = await import("node:fs"); + const p = evidencePathFor("b-warn", dir); + initEvidenceFile(p, "b-warn", 1); + setGithubEvidence(p, "org/r", { deployState: { sha: "abc1234" } }, 2); + + const raw = readFileSync(p, "utf8"); + const head = raw.slice(0, 400); + assert.match(head, /PARTIAL VIEW/, "warning must be in the first bytes a cat/head shows"); + assert.match(raw, /evidence-show\.mjs/, "must name the command that gives the real view"); + + // Markers are documentation, never data — nothing downstream should see them. + for (const doc of [readBaseFile(p), readEvidenceFile(p)]) { + assert.equal(doc._READ_ME_FIRST, undefined); + assert.equal(doc._USE_INSTEAD, undefined); + assert.equal(doc._WHY, undefined); + } + // And the real content still round-trips. + assert.equal(readEvidenceFile(p).github["org/r"].deployState.sha, "abc1234"); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("markers survive repeated writes without accumulating", async () => { + const dir = mkdtempSync(join(tmpdir(), "rca-warn2-")); + const { evidencePathFor, initEvidenceFile, setGithubEvidence } = await import("../lib/evidence-file.mjs"); + const { readFileSync } = await import("node:fs"); + const p = evidencePathFor("b-w2", dir); + initEvidenceFile(p, "b-w2", 1); + for (let i = 0; i < 3; i++) setGithubEvidence(p, `org/r${i}`, { deployState: { sha: "abc1234" } }, i + 2); + + const raw = readFileSync(p, "utf8"); + assert.equal(raw.split("_READ_ME_FIRST").length - 1, 1, "exactly one marker, not one per write"); + + rmSync(dir, { recursive: true, force: true }); +}); diff --git a/tests/loop-parallel-gather.test.mjs b/tests/loop-parallel-gather.test.mjs new file mode 100644 index 0000000..a05f56f --- /dev/null +++ b/tests/loop-parallel-gather.test.mjs @@ -0,0 +1,154 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { runRcaLoop } from "../lib/loop.mjs"; + +// A NEEDS_INFO turn's independent asks (lib/routing.mjs's routeAsk/routeAsks +// have no cross-ask state) should be gathered concurrently, not one round-trip +// at a time. This file proves both properties of that fix: the calls actually +// overlap, and the final message still assembles blocks in priority order +// regardless of which one finishes first. + +const CONFIG = { + turnCap: 6, + evidenceRouting: { + test_logs: { owner: "tfa", skip: true }, + product_code: { capability: "github" }, + infra: { capability: "infra" }, + other: { capability: "other" }, + }, +}; +const MANIFEST = { + github: { available: true, via: "gh" }, + infra: { available: true, via: "kubectl" }, +}; +const resolved = (threadId) => ({ + status: "RESOLVED", + threadId, + confidence: "high", + glimpse: { root_cause: "r", failure_type: "f", related_prs: [] }, + viewRca: "https://automation.browserstack.com/x", +}); +const delay = (ms) => new Promise((r) => setTimeout(r, ms)); + +test("independent NEEDS_INFO gathers run concurrently, not sequentially", async () => { + const events = []; + const gather = async (g) => { + events.push(`start:${g.evidenceType}`); + // The slower ask (product_code) is listed FIRST but finishes LAST. If + // gathers were sequential, infra's "start" could never appear before + // product_code's "end". + await delay(g.evidenceType === "product_code" ? 30 : 5); + events.push(`end:${g.evidenceType}`); + return `BLOCK:${g.evidenceType}`; + }; + + let calls = 0; + const submit = async () => { + calls++; + if (calls === 1) { + return { + status: "NEEDS_INFO", + threadId: "chat:1", + asks: [ + { evidenceType: "product_code", priority: "high", ask: { what: "diff" } }, + { evidenceType: "infra", priority: "medium", ask: { what: "pod status" } }, + ], + }; + } + return resolved("chat:1"); + }; + + await runRcaLoop({ + testRunId: "1", + firstMessage: "start", + submit, + config: CONFIG, + manifest: MANIFEST, + gather, + }); + + const firstEnd = events.findIndex((e) => e.startsWith("end:")); + const startsBeforeFirstEnd = events.slice(0, firstEnd).filter((e) => e.startsWith("start:")); + assert.equal( + startsBeforeFirstEnd.length, + 2, + `expected both gathers to start before either finished, got: ${events.join(", ")}`, + ); +}); + +test("gathered blocks preserve priority order in the message even when the slower ask finishes first", async () => { + const gather = async (g) => { + await delay(g.evidenceType === "product_code" ? 30 : 5); + return `BLOCK:${g.evidenceType}`; + }; + + let calls = 0; + const submits = []; + const submit = async (args) => { + calls++; + submits.push(args); + if (calls === 1) { + return { + status: "NEEDS_INFO", + threadId: "chat:2", + // Listed low-priority-first on purpose — the message must still put + // high-priority product_code ahead of low-priority infra. + asks: [ + { evidenceType: "infra", priority: "low", ask: { what: "pod status" } }, + { evidenceType: "product_code", priority: "high", ask: { what: "diff" } }, + ], + }; + } + return resolved("chat:2"); + }; + + await runRcaLoop({ + testRunId: "2", + firstMessage: "start", + submit, + config: CONFIG, + manifest: MANIFEST, + gather, + }); + + assert.equal( + submits[1].message, + "BLOCK:product_code\n\nBLOCK:infra", + "high-priority product_code must precede low-priority infra regardless of which gather resolved first", + ); +}); + +test("gap blocks still follow every gathered block, unaffected by concurrency", async () => { + const gather = async (g) => `BLOCK:${g.evidenceType}`; + + let calls = 0; + const submits = []; + const submit = async (args) => { + calls++; + submits.push(args); + if (calls === 1) { + return { + status: "NEEDS_INFO", + threadId: "chat:3", + asks: [ + { evidenceType: "product_code", priority: "high", ask: { what: "diff" } }, + { evidenceType: "metrics", priority: "low", ask: { what: "latency" } }, // no capability -> gap + ], + }; + } + return resolved("chat:3"); + }; + + const result = await runRcaLoop({ + testRunId: "3", + firstMessage: "start", + submit, + config: CONFIG, + manifest: MANIFEST, + gather, + }); + + assert.match(submits[1].message, /^BLOCK:product_code\n\nASK:/); + assert.deepEqual(result.asks_fulfilled, ["product_code"]); + assert.deepEqual(result.asks_unavailable, ["metrics"]); +}); diff --git a/tests/loop-turn1-result.test.mjs b/tests/loop-turn1-result.test.mjs new file mode 100644 index 0000000..a7d286f --- /dev/null +++ b/tests/loop-turn1-result.test.mjs @@ -0,0 +1,110 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { runRcaLoop } from "../lib/loop.mjs"; + +// Step 4b (SKILL.md Step 4b) pre-submits turn 1 for a cluster representative, +// concurrently with Step 4's evidence pre-fetch. When it lands NEEDS_INFO, +// `turn1Result` carries that thread + those asks in so the loop never +// resubmits turn 1 — this file is the conformance coverage for that skip-ahead +// path, mirroring tests/conformance.test.mjs's fixture style but inline since +// there's nothing to replay: turn 1 already happened before runRcaLoop starts. + +const CONFIG = { + turnCap: 6, + evidenceRouting: { + test_logs: { owner: "tfa", skip: true }, + product_code: { capability: "github" }, + other: { capability: "other" }, + }, +}; +const GITHUB_AVAILABLE = { github: { available: true, via: "gh" } }; +const gather = async (g) => `ASK: ${g.ask.what}\nTYPE: ${g.evidenceType}\nFOUND: yes\nSUMMARY: stub`; + +test("turn1Result: never resubmits turn 1, starts at ROUTE, turns_used counts the pre-dispatched turn", async () => { + const submits = []; + // The only submit() call the loop makes is the FOLLOW-UP — turn 1 already + // happened in Step 4b and is represented purely by `turn1Result`. + const submit = async (args) => { + submits.push(args); + return { + status: "RESOLVED", + threadId: "chat:99", + confidence: "high", + glimpse: { root_cause: "root cause found", failure_type: "product_regression", related_prs: ["#1"] }, + viewRca: "https://automation.browserstack.com/x", + }; + }; + + const result = await runRcaLoop({ + testRunId: "99", + submit, + config: CONFIG, + manifest: GITHUB_AVAILABLE, + gather, + turn1Result: { threadId: "chat:99", asks: [{ evidenceType: "product_code", ask: { what: "diff" } }] }, + }); + + assert.equal(submits.length, 1, "turn 1 must never be submitted — only the follow-up"); + assert.equal(submits[0].threadId, "chat:99", "the follow-up reuses Step 4b's thread"); + assert.equal(submits[0].turnId, undefined, "no turnId — NEEDS_INFO never carries one"); + assert.equal(result.status, "RESOLVED"); + assert.equal(result.turns_used, 2, "1 = Step 4b's pre-dispatched turn, 2 = this follow-up"); + assert.equal(result.threadId, "chat:99"); +}); + +test("turn1Result only short-circuits the FIRST pass — later iterations submit normally", async () => { + let calls = 0; + const submit = async () => { + calls++; + if (calls === 1) { + return { status: "NEEDS_INFO", threadId: "chat:99", asks: [{ evidenceType: "other", ask: { what: "logs excerpt" } }] }; + } + return { + status: "RESOLVED", + threadId: "chat:99", + confidence: "medium", + glimpse: { root_cause: "resolved on turn 3", failure_type: "infra", related_prs: [] }, + viewRca: "https://automation.browserstack.com/y", + }; + }; + + const result = await runRcaLoop({ + testRunId: "99", + submit, + config: CONFIG, + manifest: GITHUB_AVAILABLE, + gather, + turn1Result: { threadId: "chat:99", asks: [{ evidenceType: "product_code", ask: { what: "diff" } }] }, + }); + + assert.equal(calls, 2, "one real submit for the ROUTE follow-up, one more to resolve"); + assert.equal(result.status, "RESOLVED"); + assert.equal(result.turns_used, 3, "1 pre-dispatched + 2 real submits"); +}); + +test("without turn1Result, behaviour is unchanged: turn 1 IS submitted normally", async () => { + const submits = []; + const submit = async (args) => { + submits.push(args); + return { + status: "RESOLVED", + threadId: "chat:1", + confidence: "high", + glimpse: { root_cause: "root", failure_type: "product_regression", related_prs: [] }, + viewRca: "https://automation.browserstack.com/z", + }; + }; + + const result = await runRcaLoop({ + testRunId: "1", + firstMessage: "Initiating collaborative RCA for test run 1.", + submit, + config: CONFIG, + manifest: GITHUB_AVAILABLE, + gather, + }); + + assert.equal(submits.length, 1); + assert.equal(submits[0].message, "Initiating collaborative RCA for test run 1."); + assert.equal(result.turns_used, 1); +}); diff --git a/tests/repo-source.test.mjs b/tests/repo-source.test.mjs new file mode 100644 index 0000000..08cfb40 --- /dev/null +++ b/tests/repo-source.test.mjs @@ -0,0 +1,148 @@ +import { test, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, mkdirSync, writeFileSync } from "node:fs"; +import { execFileSync } from "node:child_process"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { localCloneFor, hasCommit, readFileAt, discoverWorkspaceRoot, resolveLocalRepos } from "../lib/repo-source.mjs"; + +let ws, repoDir, sha1, sha2; + +// Build a real throwaway git repo with two commits, so the staleness scenario +// is exercised for real rather than mocked. +beforeEach(() => { + ws = mkdtempSync(join(tmpdir(), "rca-ws-")); + repoDir = join(ws, "testrepo"); + mkdirSync(repoDir); + const g = (...a) => execFileSync("git", ["-C", repoDir, ...a], { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }); + g("init", "-q"); + g("config", "user.email", "t@t.t"); + g("config", "user.name", "t"); + writeFileSync(join(repoDir, "app.js"), "VERSION_ONE\n"); + g("add", "."); g("commit", "-qm", "one"); + sha1 = g("rev-parse", "HEAD").trim(); + writeFileSync(join(repoDir, "app.js"), "VERSION_TWO\n"); + g("add", "."); g("commit", "-qm", "two"); + sha2 = g("rev-parse", "HEAD").trim(); +}); +afterEach(() => rmSync(ws, { recursive: true, force: true })); + +test("localCloneFor finds a clone by bare repo name", () => { + assert.equal(localCloneFor("browserstack/testrepo", ws), repoDir); + assert.equal(localCloneFor("browserstack/not-cloned", ws), null); +}); + +test("hasCommit distinguishes present from absent commits", () => { + assert.equal(hasCommit(repoDir, sha1), true); + assert.equal(hasCommit(repoDir, "0".repeat(40)), false); +}); + +// The core safety property. A branch name resolves to whatever the clone +// happens to have, which on a real machine was 12 commits stale and returned +// different bytes than the true head — a silently wrong RCA input. +test("a branch name is REFUSED; only a commit sha is accepted", () => { + const r = readFileAt({ repo: "browserstack/testrepo", sha: "main", path: "app.js", workspaceRoot: ws }); + assert.equal(r.ok, false); + assert.equal(r.source, "remote-needed"); + assert.match(r.reason, /must be a commit sha/); +}); + +test("a pinned sha reads the content AT THAT COMMIT, not the tip", () => { + const older = readFileAt({ repo: "browserstack/testrepo", sha: sha1, path: "app.js", workspaceRoot: ws }); + assert.equal(older.ok, true); + assert.equal(older.source, "local"); + assert.equal(older.content.trim(), "VERSION_ONE", "must read the old commit, not HEAD"); + + const newer = readFileAt({ repo: "browserstack/testrepo", sha: sha2, path: "app.js", workspaceRoot: ws }); + assert.equal(newer.content.trim(), "VERSION_TWO"); +}); + +test("no local clone -> defers to the caller for a remote read", () => { + const r = readFileAt({ repo: "browserstack/absent", sha: sha1, path: "app.js", workspaceRoot: ws }); + assert.equal(r.ok, false); + assert.equal(r.source, "remote-needed"); + assert.match(r.reason, /no local clone/); +}); + +test("commit absent locally -> remote-needed, and does NOT fetch unless asked", () => { + const r = readFileAt({ repo: "browserstack/testrepo", sha: "0".repeat(40), path: "app.js", workspaceRoot: ws }); + assert.equal(r.ok, false); + assert.equal(r.source, "remote-needed"); + assert.match(r.reason, /not present|allowFetch/); +}); + +// A path that genuinely didn't exist at that commit is an ANSWER. Treating it +// as a fallback trigger would send the caller to the network to be told the +// same thing, and risks a tip-of-branch read papering over the real history. +// The real shape: the plugin lives one level inside the workspace, alongside +// the clones, so the root is found on the second try. +test("discoverWorkspaceRoot walks up to the dir holding the clones", () => { + const pluginDir = join(ws, "some-plugin"); + mkdirSync(pluginDir, { recursive: true }); + const d = discoverWorkspaceRoot({ repos: ["browserstack/testrepo"], from: pluginDir }); + assert.equal(d.root, ws); + assert.equal(d.matched, "browserstack/testrepo"); + assert.equal(d.tried.length, 2, "found on the second candidate"); +}); + +// The bound is a feature: from deep inside a repo the root is out of reach, +// and the correct answer is to stop rather than climb toward `/` and risk +// matching an unrelated checkout. +test("discoverWorkspaceRoot stops at maxTries instead of climbing far", () => { + const deep = join(repoDir, "a", "b", "c"); + mkdirSync(deep, { recursive: true }); + const d = discoverWorkspaceRoot({ repos: ["browserstack/testrepo"], from: deep, maxTries: 3 }); + assert.equal(d.root, null, "workspace is 4 levels up — out of the bounded range"); + assert.equal(d.tried.length, 3); +}); + +// Genericity: a candidate wins only if it holds a repo THIS run validated. +// Nothing about the product or layout is assumed. +test("discoverWorkspaceRoot verifies against the run's own repo list", () => { + const d = discoverWorkspaceRoot({ repos: ["browserstack/some-other-product"], from: repoDir }); + assert.equal(d.root, null, "must not accept a dir that lacks the requested repo"); + assert.match(d.reason, /some-other-product/); +}); + +test("discoverWorkspaceRoot is bounded — it gives up rather than hunting", () => { + const d = discoverWorkspaceRoot({ repos: ["browserstack/nope"], from: repoDir, maxTries: 3 }); + assert.equal(d.root, null); + assert.ok(d.tried.length <= 3, `tried ${d.tried.length}, expected <= 3`); +}); + +test("an explicit root is still VERIFIED, so a stale override fails loudly", () => { + const ok = discoverWorkspaceRoot({ repos: ["browserstack/testrepo"], explicit: ws }); + assert.equal(ok.root, ws); + const bad = discoverWorkspaceRoot({ repos: ["browserstack/testrepo"], explicit: join(ws, "nowhere") }); + assert.equal(bad.root, null, "a wrong explicit path must not be trusted blindly"); +}); + +// This is the context-saving payload: resolved once, read by every coordinator. +test("resolveLocalRepos reports per-repo usability at the pinned sha", () => { + const r = resolveLocalRepos({ + repos: ["browserstack/testrepo", "browserstack/absent"], + pins: { "browserstack/testrepo": sha1 }, + workspaceRoot: ws, + }); + assert.equal(r["browserstack/testrepo"].usable, true); + assert.equal(r["browserstack/testrepo"].sha, sha1); + assert.equal(r["browserstack/absent"].usable, false); + assert.match(r["browserstack/absent"].reason, /no local clone/); +}); + +test("resolveLocalRepos marks a repo unusable when its sha is absent", () => { + const r = resolveLocalRepos({ + repos: ["browserstack/testrepo"], + pins: { "browserstack/testrepo": "0".repeat(40) }, + workspaceRoot: ws, + }); + assert.equal(r["browserstack/testrepo"].usable, false); + assert.match(r["browserstack/testrepo"].reason, /not present locally/); +}); + +test("path missing at that commit is a local answer, not a remote fallback", () => { + const r = readFileAt({ repo: "browserstack/testrepo", sha: sha1, path: "nope.js", workspaceRoot: ws }); + assert.equal(r.ok, false); + assert.equal(r.source, "local"); + assert.match(r.reason, /path not present/); +}); diff --git a/tests/signature.test.mjs b/tests/signature.test.mjs index f721167..02dbfd0 100644 --- a/tests/signature.test.mjs +++ b/tests/signature.test.mjs @@ -1,5 +1,8 @@ import { test } from "node:test"; import assert from "node:assert/strict"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { normalize, computeSignature, @@ -77,3 +80,62 @@ test("clusterRows stamps cluster_id onto every row", () => { assert.ok(rows.every((r) => r.cluster_id)); assert.notEqual(rows[0].cluster_id, rows[1].cluster_id); }); + +// clusterRows mutates its input; a caller that destructures only `clusters` +// silently loses every cluster_id. Two independent callers did exactly that on +// the same day, collapsing a clustered run into one coordinator per test. +test("clusterAndPersist writes cluster_id back to the CSV", async () => { + const dir = mkdtempSync(join(tmpdir(), "rca-cap-")); + const csvState = await import("../lib/csv-state.mjs"); + const { clusterAndPersist } = await import("../lib/signature.mjs"); + const csv = join(dir, "s.csv"); + csvState.seed(csv, "b", [ + { test_id: 1, test_name: "a", failure: { error_summary: "boom" } }, + { test_id: 2, test_name: "b", failure: { error_summary: "boom" } }, + { test_id: 3, test_name: "c", failure: { error_summary: "other" } }, + ]); + + const clusters = clusterAndPersist(csv, csvState); + assert.equal(clusters.length, 2, "two distinct signatures"); + + // The whole point: re-READ from disk, don't trust the in-memory rows. + const reread = csvState.readRows(csv); + assert.ok(reread.every((r) => r.cluster_id), "every row must have a persisted cluster_id"); + assert.equal(reread[0].cluster_id, reread[1].cluster_id, "same signature → same cluster"); + assert.notEqual(reread[0].cluster_id, reread[2].cluster_id); + + rmSync(dir, { recursive: true, force: true }); +}); + +// Siblings are only cheap because they confirm someone else's hypothesis. +// Dispatched without one they re-investigate from scratch — measured at 22.7 +// calls vs 8.0 for the representative they were meant to be a fraction of. +test("siblingPreSeed refuses to seed from an unfinished representative", async () => { + const dir = mkdtempSync(join(tmpdir(), "rca-seed-")); + const csvState = await import("../lib/csv-state.mjs"); + const { siblingPreSeed } = await import("../lib/signature.mjs"); + const csv = join(dir, "s.csv"); + csvState.seed(csv, "b", [ + { test_id: 1, test_name: "rep", failure: { error_summary: "boom" } }, + { test_id: 2, test_name: "sib", failure: { error_summary: "boom" } }, + ]); + + const early = siblingPreSeed(csv, csvState, "c-1", 1); + assert.equal(early.ok, false, "rep is still pending — must block"); + assert.match(early.reason, /not resolved/); + + // Resolved but with no root_cause is equally useless to a sibling. + csvState.flip(csv, 1, { rca_done: "resolved" }, 1000); + const empty = siblingPreSeed(csv, csvState, "c-1", 1); + assert.equal(empty.ok, false); + assert.match(empty.reason, /no root_cause/); + + csvState.flip(csv, 1, { rca_done: "resolved", root_cause: "PR #42 broke seeding", failure_type: "PRODUCT_BUG" }, 2000); + const ok = siblingPreSeed(csv, csvState, "c-1", 1); + assert.equal(ok.ok, true); + assert.equal(ok.pre_seed.cause, "PR #42 broke seeding"); + assert.equal(ok.pre_seed.failure_type, "PRODUCT_BUG"); + assert.match(ok.pre_seed.instruction, /Do not adopt it/, "independence must travel with the seed"); + + rmSync(dir, { recursive: true, force: true }); +}); diff --git a/tests/state-dir.test.mjs b/tests/state-dir.test.mjs new file mode 100644 index 0000000..1613785 --- /dev/null +++ b/tests/state-dir.test.mjs @@ -0,0 +1,102 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, mkdirSync, writeFileSync, chmodSync, statSync, existsSync, utimesSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { hardenStateDir, pruneStateDir } from "../lib/state-dir.mjs"; + +const mode = (p) => statSync(p).mode & 0o777; + +function fixture() { + const root = mkdtempSync(join(tmpdir(), "rca-sd-")); + const dir = join(root, "bstack-rca"); + mkdirSync(dir, { mode: 0o755 }); + writeFileSync(join(dir, "rca-state.b1.csv"), "a\n", { mode: 0o644 }); + const cache = join(dir, "rca-toolcache.b1"); + mkdirSync(cache, { mode: 0o755 }); + writeFileSync(join(cache, "entry.json"), "{}", { mode: 0o644 }); + chmodSync(dir, 0o755); + chmodSync(cache, 0o755); + return { root, dir, cache }; +} + +// Per-write hardening only fixes the file being written, so a build analysed +// before the hardening landed keeps 0644 forever — a completed build is never +// rewritten. This is the sweep that repairs them. +test("hardenStateDir tightens leftovers recursively", () => { + const { root, dir, cache } = fixture(); + assert.equal(mode(dir), 0o755, "fixture must start open, else the test proves nothing"); + + const r = hardenStateDir(dir); + + assert.equal(mode(dir), 0o700); + assert.equal(mode(join(dir, "rca-state.b1.csv")), 0o600); + assert.equal(mode(cache), 0o700, "nested cache dir too"); + assert.equal(mode(join(cache, "entry.json")), 0o600, "files inside nested dirs too"); + assert.equal(r.files, 2); + assert.equal(r.dirs, 2); + + rmSync(root, { recursive: true, force: true }); +}); + +test("hardenStateDir is idempotent and safe on a missing dir", () => { + const { root, dir } = fixture(); + hardenStateDir(dir); + const second = hardenStateDir(dir); + assert.equal(mode(dir), 0o700); + assert.equal(second.files, 2, "still walks, just has nothing to change"); + + assert.deepEqual(hardenStateDir(join(root, "nope")), { dirs: 0, files: 0, skipped: [] }); + rmSync(root, { recursive: true, force: true }); +}); + +// These files ARE the resume state, so the default must not be able to eat a +// build someone is about to resume. +test("pruneStateDir keeps recent artifacts and removes only old ones", () => { + const { root, dir } = fixture(); + // Pin every fixture file's mtime relative to the test's own clock — the + // files are created at real "now", so a hardcoded future nowMs would make + // the whole fixture look ancient and the test would pass for the wrong + // reason (it did, first time round). + const now = 1_800_000_000_000; + // utimesSync takes SECONDS as a plain number; `new Date(seconds)` would be + // read as milliseconds and land every stamp in 1970. + const stamp = (p, ageSec) => { const s = now / 1000 - ageSec; utimesSync(p, s, s); }; + stamp(join(dir, "rca-state.b1.csv"), 60); + stamp(join(dir, "rca-toolcache.b1"), 60); + const old = join(dir, "rca-state.ancient.csv"); + writeFileSync(old, "x\n"); + stamp(old, 8 * 24 * 60 * 60); + + const r = pruneStateDir(dir, now); + + assert.deepEqual(r.removed, ["rca-state.ancient.csv"]); + assert.equal(existsSync(old), false); + assert.ok(existsSync(join(dir, "rca-state.b1.csv")), "a fresh build must survive"); + assert.ok(r.kept >= 1); + + rmSync(root, { recursive: true, force: true }); +}); + +test("pruneStateDir dryRun reports without deleting", () => { + const { root, dir } = fixture(); + // Pin every fixture file's mtime relative to the test's own clock — the + // files are created at real "now", so a hardcoded future nowMs would make + // the whole fixture look ancient and the test would pass for the wrong + // reason (it did, first time round). + const now = 1_800_000_000_000; + // utimesSync takes SECONDS as a plain number; `new Date(seconds)` would be + // read as milliseconds and land every stamp in 1970. + const stamp = (p, ageSec) => { const s = now / 1000 - ageSec; utimesSync(p, s, s); }; + stamp(join(dir, "rca-state.b1.csv"), 60); + stamp(join(dir, "rca-toolcache.b1"), 60); + const old = join(dir, "rca-state.ancient.csv"); + writeFileSync(old, "x\n"); + stamp(old, 8 * 24 * 60 * 60); + + const r = pruneStateDir(dir, now, { dryRun: true }); + assert.deepEqual(r.removed, ["rca-state.ancient.csv"]); + assert.equal(existsSync(old), true, "dryRun must not delete"); + + rmSync(root, { recursive: true, force: true }); +}); diff --git a/tests/theme-clustering.test.mjs b/tests/theme-clustering.test.mjs new file mode 100644 index 0000000..eb1004b --- /dev/null +++ b/tests/theme-clustering.test.mjs @@ -0,0 +1,168 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { clustersFromThemes } from "../lib/theme-clustering.mjs"; + +function row(id, extra = {}) { + return { + testRunId: String(id), + failure_category: "Assertion", + error_summary: "expected 200 but got 500", + file_path: "spec/login.rb", + is_flaky: "false", + ...extra, + }; +} + +function theme(buildFailureThemeId, name = "Some Theme") { + return { + themeId: `uuid-${buildFailureThemeId}`, + buildFailureThemeId, + themeData: { name, description: "..." }, + affectedWorkflows: [], + }; +} + +test("one theme with two members → one cluster, representative + one sibling", () => { + const rows = [row(1), row(2)]; + const themesResult = { buildThemes: [theme(10, "Data Assertion Mismatch")] }; + const testsByThemeId = { 10: [{ testRunId: "1" }, { testRunId: "2" }] }; + + const { clusters } = clustersFromThemes(rows, themesResult, testsByThemeId); + + assert.equal(clusters.length, 1); + assert.equal(clusters[0].cluster_id, "theme-10"); + assert.equal(clusters[0].signature, "Data Assertion Mismatch"); + assert.equal(clusters[0].members.length, 2); + assert.equal(clusters[0].siblings.length, 1); + assert.ok(clusters[0].representative); +}); + +test("multiple themes → one cluster each", () => { + const rows = [row(1), row(2), row(3)]; + const themesResult = { + buildThemes: [theme(10, "Theme A"), theme(20, "Theme B")], + }; + const testsByThemeId = { + 10: [{ testRunId: "1" }], + 20: [{ testRunId: "2" }, { testRunId: "3" }], + }; + + const { clusters } = clustersFromThemes(rows, themesResult, testsByThemeId); + + assert.equal(clusters.length, 2); + const a = clusters.find((c) => c.cluster_id === "theme-10"); + const b = clusters.find((c) => c.cluster_id === "theme-20"); + assert.equal(a.members.length, 1); + assert.equal(a.siblings.length, 0); + assert.equal(b.members.length, 2); + assert.equal(b.siblings.length, 1); +}); + +test("a failed test not assigned to any theme becomes its own singleton (never dropped)", () => { + const rows = [row(1), row(2)]; + const themesResult = { buildThemes: [theme(10, "Theme A")] }; + const testsByThemeId = { 10: [{ testRunId: "1" }] }; + + const { clusters } = clustersFromThemes(rows, themesResult, testsByThemeId); + + assert.equal(clusters.length, 2); + const solo = clusters.find((c) => c.cluster_id === "solo-2"); + assert.ok(solo, "uncovered test must still get a cluster"); + assert.equal(solo.members.length, 1); + assert.equal(solo.siblings.length, 0); + assert.equal(solo.representative.testRunId, "2"); +}); + +test("a theme with no matched member rows is skipped, not an empty cluster", () => { + const rows = [row(1)]; + const themesResult = { buildThemes: [theme(10, "Theme A"), theme(20, "Empty theme")] }; + const testsByThemeId = { 10: [{ testRunId: "1" }], 20: [] }; + + const { clusters } = clustersFromThemes(rows, themesResult, testsByThemeId); + + assert.equal(clusters.length, 1); + assert.equal(clusters[0].cluster_id, "theme-10"); +}); + +test("member rows not present in listTestIds rows are dropped, not fabricated", () => { + const rows = [row(1)]; + const themesResult = { buildThemes: [theme(10, "Theme A")] }; + // testsByThemeId names a testRunId ("999") that never appeared in listTestIds. + const testsByThemeId = { 10: [{ testRunId: "1" }, { testRunId: "999" }] }; + + const { clusters } = clustersFromThemes(rows, themesResult, testsByThemeId); + + assert.equal(clusters.length, 1); + assert.equal(clusters[0].members.length, 1); +}); + +test("representative selection matches lib/signature.mjs's rule (non-flaky, then smallest testRunId)", () => { + const rows = [ + row(5, { is_flaky: "true" }), + row(9, { is_flaky: "false" }), + row(7, { is_flaky: "false" }), + ]; + const themesResult = { buildThemes: [theme(10)] }; + const testsByThemeId = { + 10: [{ testRunId: "5" }, { testRunId: "9" }, { testRunId: "7" }], + }; + + const { clusters } = clustersFromThemes(rows, themesResult, testsByThemeId); + + assert.equal(clusters[0].representative.testRunId, "7"); +}); + +test("clustersFromThemes stamps cluster_id onto every row, theme and singleton alike", () => { + const rows = [row(1), row(2)]; + const themesResult = { buildThemes: [theme(10)] }; + const testsByThemeId = { 10: [{ testRunId: "1" }] }; + + clustersFromThemes(rows, themesResult, testsByThemeId); + + assert.equal(rows[0].cluster_id, "theme-10"); + assert.equal(rows[1].cluster_id, "solo-2"); +}); + +test("numeric testRunId in theme membership (as the MCP tool's JSON would send it) still matches string testRunId rows", () => { + const rows = [row(1), row(2)]; + const themesResult = { buildThemes: [theme(10)] }; + // Membership entries carry testRunId as a NUMBER, unlike listTestIds rows (strings). + const testsByThemeId = { 10: [{ testRunId: 1 }, { testRunId: 2 }] }; + + const { clusters } = clustersFromThemes(rows, themesResult, testsByThemeId); + + assert.equal(clusters.length, 1); + assert.equal(clusters[0].members.length, 2); +}); + +test("a testRunId claimed by an earlier theme is skipped by a later theme (first-theme-wins, no duplicate membership)", () => { + const rows = [row(1), row(2)]; + const themesResult = { buildThemes: [theme(10, "Theme A"), theme(20, "Theme B")] }; + // Both themes claim testRunId "1" — the server is expected never to do this, + // but the function must not let the row land in two clusters. + const testsByThemeId = { + 10: [{ testRunId: "1" }], + 20: [{ testRunId: "1" }, { testRunId: "2" }], + }; + + const { clusters } = clustersFromThemes(rows, themesResult, testsByThemeId); + + assert.equal(clusters.length, 2); + const a = clusters.find((c) => c.cluster_id === "theme-10"); + const b = clusters.find((c) => c.cluster_id === "theme-20"); + assert.equal(a.members.length, 1); + assert.equal(a.members[0].testRunId, "1"); + assert.equal(b.members.length, 1, "testRunId 1 must not also land in theme 20"); + assert.equal(b.members[0].testRunId, "2"); + assert.equal(rows[0].cluster_id, "theme-10"); +}); + +test("no themes at all → every row is its own singleton", () => { + const rows = [row(1), row(2)]; + const themesResult = { buildThemes: [] }; + + const { clusters } = clustersFromThemes(rows, themesResult, {}); + + assert.equal(clusters.length, 2); + assert.ok(clusters.every((c) => c.cluster_id.startsWith("solo-"))); +}); diff --git a/tests/tool-cache.test.mjs b/tests/tool-cache.test.mjs new file mode 100644 index 0000000..56e22da --- /dev/null +++ b/tests/tool-cache.test.mjs @@ -0,0 +1,272 @@ +import { test, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, writeFileSync, statSync, readdirSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + toolCacheDirFor, cacheKey, mcpCacheKey, cacheGet, cachePut, cacheStats, + isCacheable, isCacheableMcp, isRunnable, redact, tokenize, splitPipeline, +} from "../lib/tool-cache.mjs"; + +let dir; +beforeEach(() => { dir = mkdtempSync(join(tmpdir(), "rca-toolcache-")); }); +afterEach(() => rmSync(dir, { recursive: true, force: true })); + +test("toolCacheDirFor: build id in the path, OS temp default, stateDir override", () => { + assert.ok(toolCacheDirFor("b1").startsWith(join(tmpdir(), "bstack-rca"))); + assert.ok(toolCacheDirFor("b1").endsWith("rca-toolcache.b1")); + assert.equal(toolCacheDirFor("b1", "/ci/art"), join("/ci/art", "rca-toolcache.b1")); + assert.ok(toolCacheDirFor("../../etc").endsWith("rca-toolcache..._.._etc")); +}); + +test("cacheKey: whitespace-insensitive, but content-sensitive", () => { + assert.equal(cacheKey("gh api repos/a"), cacheKey("gh api repos/a")); + assert.notEqual(cacheKey("gh api repos/a | head -20"), cacheKey("gh api repos/a | head -200")); +}); + +test("put then get round-trips", () => { + const k = cacheKey("gh api repos/a"); + cachePut(dir, k, { command: "gh api repos/a", writerId: "w1", stdout: "hello" }, 1000); + const hit = cacheGet(dir, k); + assert.equal(hit.stdout, "hello"); + assert.equal(hit.writerId, "w1"); + assert.equal(hit.capturedAtMs, 1000); +}); + +test("get on a miss returns null, never throws", () => { + assert.equal(cacheGet(dir, cacheKey("never run")), null); +}); + +test("a corrupt entry reads as a miss rather than throwing", () => { + const k = cacheKey("gh api repos/a"); + writeFileSync(join(dir, `${k}.json`), "{not json", "utf8"); + assert.equal(cacheGet(dir, k), null); +}); + +test("secrets are redacted before anything is written to disk", () => { + const k = cacheKey("gh api repos/a"); + cachePut(dir, k, { + command: "gh api repos/a", + stdout: 'ok\nAuthorization: Bearer abc123SECRET\napi_key=zzz999\ndone', + }, 1000); + const raw = cacheGet(dir, k).stdout; + assert.ok(!raw.includes("abc123SECRET"), "bearer token must not persist"); + assert.ok(!raw.includes("zzz999"), "api_key must not persist"); + assert.ok(raw.includes("<redacted>")); +}); + +test("redact leaves ordinary output untouched", () => { + assert.equal(redact("just some log output"), "just some log output"); +}); + +// Regression, found by a live coordinator. Redaction used to consume the REST +// OF THE LINE after a secret-ish key. GitHub's file API returns SINGLE-LINE +// JSON whose download_url always carries `?token=…`, so a 214KB response was +// silently cached as 816 bytes with the content field gone — every +// private-repo file fetch was corrupted, with no warning. +test("redact bounds the value and does NOT eat the rest of a single-line JSON", () => { + const json = '{"name":"F.java","download_url":"https://raw.example/F.java?token=BRFIJBHPIG5IILHZ",' + + '"type":"file","content":"' + "A".repeat(5000) + '"}'; + const out = redact(json); + assert.ok(!out.includes("BRFIJBHPIG5IILHZ"), "the token itself must be redacted"); + assert.ok(out.includes('"type":"file"'), "structure after the token must survive"); + assert.ok(out.includes("A".repeat(5000)), "the content payload must survive"); + assert.ok(out.length > 5000, `expected full payload, got ${out.length} bytes`); +}); + +test("redact still catches a bare Bearer token and a key=value secret", () => { + assert.equal(redact("Authorization: Bearer abc123SECRET"), "Authorization: <redacted>"); + assert.equal(redact("api_key=zzz999"), "api_key=<redacted>"); + assert.ok(!redact("Bearer eyJhbGciOiJIUzI1NiJ9").includes("eyJhbGciOiJIUzI1NiJ9")); +}); + +// Regression, found by two live coordinators. Neither tokenize nor +// splitPipeline handled backslash escapes, so `\"` read as a closing quote. +// That mangled jq's two most common idioms: string equality and, because the +// parser then believed it was outside quotes, regex alternation got split as +// a shell pipe. +test("escaped double quotes survive tokenization for jq", () => { + const argv = tokenize(String.raw`gh api x --jq .[]|select(.filename==\"a/b.json\")`); + assert.equal(argv[argv.length - 1], '.[]|select(.filename=="a/b.json")'); +}); + +test("a pipe inside an escaped-quote jq regex is NOT a shell pipe", () => { + const cmd = String.raw`gh pr view 51044 --json files | jq -c "select(test(\"vite|env|s3\";\"i\"))"`; + assert.deepEqual(splitPipeline(cmd).length, 2, "must split into fetch + one filter only"); + const g = isRunnable(cmd); + assert.equal(g.ok, true, g.reason); + assert.deepEqual(g.fetch, ["gh", "pr", "view", "51044", "--json", "files"]); + assert.equal(g.filters[0][2], 'select(test("vite|env|s3";"i"))'); +}); + +test("single quotes suppress escape processing, POSIX-style", () => { + assert.deepEqual(tokenize(String.raw`gh api 'a\nb'`), ["gh", "api", String.raw`a\nb`]); +}); + +test("oversized payloads are truncated and flagged", () => { + const k = cacheKey("gh api big"); + const rec = cachePut(dir, k, { command: "gh api big", stdout: "x".repeat(400 * 1024) }, 1000); + assert.equal(rec.truncated, true); + assert.ok(rec.stdout.includes("[truncated by tool-cache]")); +}); + +test("cacheStats counts entries", () => { + cachePut(dir, "k1", { command: "a", stdout: "12345" }, 1); + cachePut(dir, "k2", { command: "b", stdout: "123" }, 1); + const s = cacheStats(dir); + assert.equal(s.entries, 2); + assert.equal(s.bytes, 8); +}); + +test("isCacheable rejects mutating shell commands", () => { + assert.equal(isCacheable("gh api repos/a"), true); + assert.equal(isCacheable("kubectl get pods"), true); + assert.equal(isCacheable("kubectl delete pod x"), false); + assert.equal(isCacheable("kubectl exec pod -- sh"), false); + assert.equal(isCacheable("gh pr create --title x"), false); + assert.equal(isCacheable("gh api -X POST repos/a"), false); + assert.equal(isCacheable("git push origin main"), false); + assert.equal(isCacheable("rm -rf /tmp/x"), false); +}); + +test("isRunnable enforces an allowlisted read-only leader", () => { + assert.equal(isRunnable("gh api repos/a").ok, true); + assert.equal(isRunnable("kubectl get pods -n regression").ok, true); + assert.equal(isRunnable("python3 -c 'print(1)'").ok, false); + assert.equal(isRunnable("sh -c 'echo hi'").ok, false); +}); + +test("isRunnable rejects chaining and redirects, but ACCEPTS pipelines", () => { + assert.equal(isRunnable("gh api a ; rm -rf /").ok, false); + assert.equal(isRunnable("gh api a && kubectl delete pod x").ok, false); + assert.equal(isRunnable("gh api a > /etc/passwd").ok, false); + // `2>&1` is stderr plumbing the wrapper already owns — stripped, not refused. + // Refusing it rejected 134 of 223 real recorded calls and zeroed the hit rate. + assert.equal(isRunnable("gh api a 2>&1").ok, true, "stderr plumbing is normalized away"); + assert.equal(isRunnable("gh api a 2>/dev/null | jq .x").ok, true); + // Pipelines are supported now: refusing them meant the cache applied to + // almost no real traffic, since most fetches are written inline with a filter. + assert.equal(isRunnable("gh api a | jq .x").ok, true); +}); + +test("a FILE redirect is reported as a redirect, not as a mutation", () => { + const r = isRunnable("gh api repos/x > out.json"); + assert.equal(r.ok, false); + assert.match(r.reason, /redirect/i); + assert.doesNotMatch(r.reason, /mutating/i); +}); + +test("stderr plumbing does not change the cache key", () => { + const a = isRunnable("gh api repos/x | jq .a"); + const b = isRunnable("gh api repos/x 2>&1 | jq .b"); + assert.equal(cacheKey(a.fetchText), cacheKey(b.fetchText)); +}); + +test("pipeline plan: only the FETCH is keyed, filters are separate", () => { + const a = isRunnable("gh api repos/x | jq -r .name"); + const b = isRunnable("gh api repos/x | jq -r .branch | tr a-z A-Z"); + assert.equal(a.ok && b.ok, true); + // Same underlying fetch -> same cache key -> one network call serves both. + assert.equal(cacheKey(a.fetchText), cacheKey(b.fetchText)); + assert.deepEqual(a.fetch, ["gh", "api", "repos/x"]); + assert.equal(a.filters.length, 1); + assert.equal(b.filters.length, 2); +}); + +test("only pure text filters may follow the fetch", () => { + assert.equal(isRunnable("gh api repos/x | jq .a").ok, true); + assert.equal(isRunnable("gh api repos/x | grep foo").ok, true); + assert.equal(isRunnable("gh api repos/x | sh").ok, false); + assert.equal(isRunnable("gh api repos/x | bash -c 'x'").ok, false); + assert.equal(isRunnable("gh api repos/x | kubectl delete pod y").ok, false); +}); + +test("splitPipeline ignores a pipe inside quotes", () => { + assert.deepEqual(splitPipeline(`gh pr list --jq '.[] | .number' | head -5`), + ["gh pr list --jq '.[] | .number'", "head -5"]); +}); + +// Regression: the old raw-string guard refused these legitimate read-only +// calls, which is what pushed a coordinator into slower workarounds. +test("isRunnable ALLOWS metacharacters inside quoted arguments", () => { + const jqSemicolon = `gh api repos/o/r/git/trees/main --jq '[.tree[].path|select(test("rcaThree";"i"))]'`; + assert.equal(isRunnable(jqSemicolon).ok, true, "; inside a jq expression is not a shell operator"); + + const urlAmp = "gh api 'search/code?q=foo&per_page=20'"; + assert.equal(isRunnable(urlAmp).ok, true, "& inside a quoted URL is not a shell operator"); + + const jqPipe = `gh pr list -R o/r --json number --jq '.[] | .number'`; + assert.equal(isRunnable(jqPipe).ok, true, "| inside a quoted jq expression is not a shell pipe"); +}); + +test("a quoted metacharacter survives tokenization as ONE literal argument", () => { + const argv = tokenize(`gh api repos/o/r --jq '[.tree[]|select(test("x";"i"))]'`); + assert.equal(argv.length, 5); + assert.equal(argv[4], '[.tree[]|select(test("x";"i"))]'); +}); + +test("tokenize splits like a shell for quoted args, without a shell", () => { + assert.deepEqual(tokenize("gh api repos/a --jq '.items[].path'"), + ["gh", "api", "repos/a", "--jq", ".items[].path"]); + assert.deepEqual(tokenize('kubectl get pods -o "custom:.metadata.name"'), + ["kubectl", "get", "pods", "-o", "custom:.metadata.name"]); + assert.throws(() => tokenize("gh api 'unterminated"), /unterminated quote/); +}); + +test("tokenize keeps injection payloads as ONE literal argument", () => { + // With execFile + these argv, no shell ever sees the metacharacters. + const argv = tokenize(`gh api "repos/a;rm -rf /"`); + assert.deepEqual(argv, ["gh", "api", "repos/a;rm -rf /"]); +}); + +test("MCP: stateful tools are never cacheable", () => { + assert.equal(isCacheableMcp("mcp__grafana__query_loki_logs"), true); + assert.equal(isCacheableMcp("mcp__browserstack__listTestIds"), true); + assert.equal(isCacheableMcp("mcp__browserstack__tfaRcaTurn"), false); + assert.equal(isCacheableMcp("mcp__browserstack__getTfaTurnResult"), false); + assert.equal(isCacheableMcp("mcp__browserstack__triggerRcaReport"), false); +}); + +test("mcpCacheKey is argument-order independent but value sensitive", () => { + const a = mcpCacheKey("t", { b: 2, a: 1 }); + const b = mcpCacheKey("t", { a: 1, b: 2 }); + assert.equal(a, b); + assert.notEqual(a, mcpCacheKey("t", { a: 1, b: 3 })); + assert.notEqual(a, mcpCacheKey("other", { a: 1, b: 2 })); +}); + +test("mcpCacheKey canonicalizes nested objects and arrays", () => { + assert.equal( + mcpCacheKey("t", { q: { z: 1, y: [{ n: 1, m: 2 }] } }), + mcpCacheKey("t", { q: { y: [{ m: 2, n: 1 }], z: 1 } }), + ); +}); + +test("an MCP result round-trips through the shared store", () => { + const k = mcpCacheKey("mcp__grafana__query_loki_logs", { ns: "regression", limit: 50 }); + cachePut(dir, k, { command: "grafana query", writerId: "3889074893", stdout: "0 rows, clean" }, 1000); + assert.equal(cacheGet(dir, k).stdout, "0 rows, clean"); +}); + +test("cache files are owner-only (0600) and the dir owner-only (0700)", () => { + const sub = join(dir, "nested-cache"); + const k = cacheKey("gh api repos/a"); + cachePut(sub, k, { command: "gh api repos/a", stdout: "private repo source" }, 1000); + // The cache sits in a world-readable OS temp dir and holds raw gh/kubectl + // output; redaction is best-effort, so the mode is the real control. + assert.equal(statSync(join(sub, `${k}.json`)).mode & 0o777, 0o600); + assert.equal(statSync(sub).mode & 0o777, 0o700); +}); + +test("no temp file is left behind after an atomic put", () => { + const k = cacheKey("gh api repos/a"); + cachePut(dir, k, { command: "gh api repos/a", stdout: "x" }, 1000); + assert.deepEqual(readdirSync(dir).filter((f) => f.endsWith(".tmp")), []); +}); + +test("CONCURRENCY: same key written twice stays readable and consistent", () => { + const k = cacheKey("gh api repos/a"); + cachePut(dir, k, { command: "gh api repos/a", writerId: "w1", stdout: "same-bytes" }, 1000); + cachePut(dir, k, { command: "gh api repos/a", writerId: "w2", stdout: "same-bytes" }, 2000); + assert.equal(cacheGet(dir, k).stdout, "same-bytes"); +}); diff --git a/tests/turn1-registry.test.mjs b/tests/turn1-registry.test.mjs new file mode 100644 index 0000000..ab053f9 --- /dev/null +++ b/tests/turn1-registry.test.mjs @@ -0,0 +1,128 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, statSync, existsSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + turn1PathFor, + initTurn1Registry, + recordTurn1, + readTurn1, + readAllTurn1, + deleteTurn1Registry, +} from "../lib/turn1-registry.mjs"; + +const mode = (p) => statSync(p).mode & 0o777; + +function fixture() { + return mkdtempSync(join(tmpdir(), "rca-t1-")); +} + +test("turn1PathFor keys the file by buildId under the given stateDir", () => { + const dir = fixture(); + const p = turn1PathFor("build-123", dir); + assert.equal(p, join(dir, "rca-turn1.build-123.json")); + rmSync(dir, { recursive: true, force: true }); +}); + +test("readTurn1 on a non-existent registry returns null, not a throw", () => { + const dir = fixture(); + const p = turn1PathFor("b1", dir); + assert.equal(readTurn1(p, "3900000001"), null); + rmSync(dir, { recursive: true, force: true }); +}); + +test("initTurn1Registry creates the file and is idempotent (never clobbers prior entries)", () => { + const dir = fixture(); + const p = turn1PathFor("b1", dir); + initTurn1Registry(p, "b1", 1000); + assert.ok(existsSync(p)); + + recordTurn1(p, "3900000001", { status: "NEEDS_INFO", threadId: "chat:1", asks: ["a"] }, 2000); + // Re-init after entries exist must leave them untouched. + initTurn1Registry(p, "b1", 3000); + assert.deepEqual(readTurn1(p, "3900000001").asks, ["a"]); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("recordTurn1 + readTurn1 round-trip a PENDING entry", () => { + const dir = fixture(); + const p = turn1PathFor("b1", dir); + recordTurn1(p, "3900000002", { status: "PENDING", threadId: "chat:2", turnId: "t-2" }, 5000); + + const entry = readTurn1(p, "3900000002"); + assert.equal(entry.status, "PENDING"); + assert.equal(entry.threadId, "chat:2"); + assert.equal(entry.turnId, "t-2"); + assert.equal(entry.submittedAtMs, 5000); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("recordTurn1 + readTurn1 round-trip a NEEDS_INFO entry", () => { + const dir = fixture(); + const p = turn1PathFor("b1", dir); + recordTurn1( + p, + "3900000003", + { status: "NEEDS_INFO", threadId: "chat:3", asks: [{ evidenceType: "product_code" }] }, + 6000, + ); + + const entry = readTurn1(p, "3900000003"); + assert.equal(entry.status, "NEEDS_INFO"); + assert.equal(entry.threadId, "chat:3"); + assert.deepEqual(entry.asks, [{ evidenceType: "product_code" }]); + assert.equal(entry.turnId, undefined, "NEEDS_INFO never carries a turnId"); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("readAllTurn1 returns every recorded entry keyed by testRunId", () => { + const dir = fixture(); + const p = turn1PathFor("b1", dir); + recordTurn1(p, "1", { status: "PENDING", threadId: "chat:1", turnId: "t-1" }, 1000); + recordTurn1(p, "2", { status: "NEEDS_INFO", threadId: "chat:2", asks: [] }, 2000); + + const all = readAllTurn1(p); + assert.deepEqual(Object.keys(all).sort(), ["1", "2"]); + assert.equal(all["1"].status, "PENDING"); + assert.equal(all["2"].status, "NEEDS_INFO"); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("recordTurn1 for a second testRunId does not clobber the first", () => { + const dir = fixture(); + const p = turn1PathFor("b1", dir); + recordTurn1(p, "1", { status: "PENDING", threadId: "chat:1", turnId: "t-1" }, 1000); + recordTurn1(p, "2", { status: "PENDING", threadId: "chat:2", turnId: "t-2" }, 2000); + + assert.equal(readTurn1(p, "1").threadId, "chat:1"); + assert.equal(readTurn1(p, "2").threadId, "chat:2"); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("the registry file and its directory are owner-only (0600 / 0700)", () => { + const dir = fixture(); + const p = turn1PathFor("b1", dir); + recordTurn1(p, "1", { status: "PENDING", threadId: "chat:1", turnId: "t-1" }, 1000); + + assert.equal(mode(p), 0o600); + + rmSync(dir, { recursive: true, force: true }); +}); + +test("deleteTurn1Registry removes the file and reports whether it existed", () => { + const dir = fixture(); + const p = turn1PathFor("b1", dir); + assert.equal(deleteTurn1Registry(p), false, "nothing to delete yet"); + + recordTurn1(p, "1", { status: "PENDING", threadId: "chat:1", turnId: "t-1" }, 1000); + assert.equal(deleteTurn1Registry(p), true); + assert.equal(existsSync(p), false); + + rmSync(dir, { recursive: true, force: true }); +}); diff --git a/tests/wiring.test.mjs b/tests/wiring.test.mjs new file mode 100644 index 0000000..4138bf6 --- /dev/null +++ b/tests/wiring.test.mjs @@ -0,0 +1,117 @@ +// Guard against SHIPPED-BUT-UNREACHABLE code. +// +// Twice now a helper was built, unit-tested, and manually verified — and then +// invoked by nothing. `bin/repo-read.mjs` and `bin/evidence-show.mjs` were both +// referenced in zero skill/agent files, so at runtime every coordinator kept +// doing the expensive thing the helper existed to avoid. Unit tests can't catch +// this: the module works perfectly in isolation, which is exactly why the gap +// survives review. +// +// The prompt layer IS the call graph here. An agent only runs what its skill or +// agent markdown names, so "is this string mentioned in a prompt file" is the +// real reachability test, crude as it looks. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { readdirSync, readFileSync, statSync } from "node:fs"; +import { join } from "node:path"; + +const ROOT = new URL("..", import.meta.url).pathname; + +/** Every .md under the dirs an agent actually reads. */ +function promptText() { + const out = []; + const walk = (dir) => { + let entries; + try { entries = readdirSync(dir); } catch { return; } + for (const e of entries) { + const p = join(dir, e); + if (statSync(p).isDirectory()) walk(p); + else if (e.endsWith(".md")) out.push(readFileSync(p, "utf8")); + } + }; + for (const d of ["skills", "agents", "workflows", ".claude"]) walk(join(ROOT, d)); + return out.join("\n"); +} + +test("every bin/ helper is named by at least one prompt file", () => { + const prompts = promptText(); + const helpers = readdirSync(join(ROOT, "bin")).filter((f) => f.endsWith(".mjs")); + assert.ok(helpers.length > 0, "expected some helpers to check"); + + const orphans = helpers.filter((h) => !prompts.includes(h)); + assert.deepEqual( + orphans, + [], + `unreachable helper(s): ${orphans.join(", ")}. A helper no skill or agent ` + + `names will never run — either reference it from the prompt layer or delete it.`, + ); +}); + +// The exported-but-uncalled variant of the same bug: lib functions that exist +// only because a test calls them. Checked for the few whose whole purpose is to +// be driven by the gate, where being uncalled means the feature is off. +test("gate-critical lib exports are actually invoked outside tests", () => { + const prompts = promptText(); + const src = []; + const walk = (dir) => { + for (const e of readdirSync(dir)) { + const p = join(dir, e); + if (statSync(p).isDirectory()) walk(p); + else if (e.endsWith(".mjs")) src.push(readFileSync(p, "utf8")); + } + }; + walk(join(ROOT, "lib")); + walk(join(ROOT, "bin")); + const haystack = src.join("\n") + "\n" + prompts; + + // Each of these is a no-op unless something drives it: discovery that is + // never run means every read falls back to the network, and a map that is + // never written means every coordinator re-probes the filesystem. + for (const fn of ["discoverWorkspaceRoot", "resolveLocalRepos", "setLocalRepos", "recomputeCoverage"]) { + // Definition line doesn't count as a call site. + const uses = haystack.split(fn).length - 1; + assert.ok(uses >= 2, `${fn} appears ${uses}x outside tests — defined but never driven`); + } +}); + +// The root cause of the 23% discovery tax was DRIFT: helpers were added faster +// than the docs described them, so agents grepped lib/ at runtime to learn the +// API. Documenting it once fixes today; this test keeps it fixed. +test("every exported lib helper appears in the SKILL's API reference", () => { + const skill = readFileSync(join(ROOT, "skills/rca-build/SKILL.md"), "utf8"); + + // Internal-by-convention: replay/test seams and trivial helpers a coordinator + // never calls. Anything NOT listed here must be documented. + const INTERNAL = new Set([ + "emptyEvidenceFile", "writeEvidenceFile", "contribDirFor", "contribPathFor", + "hasTrustworthyPrList", "stalenessOf", "makeEvidenceCache", + "replaySubmit", "replayRead", "normalize", "computeSignature", + "selectRepresentative", "localCloneFor", "hasCommit", "ensureCommit", + "classifyCoverage", "coverageStamp", "orderAsks", "routeAsk", + "unavailableCapabilities", "renderGlimpse", "toolCacheDirFor", "cacheKey", + "isCacheable", "splitPipeline", + // tool-cache module internals — agents drive the cache through + // bin/cached-exec.mjs / bin/cached-mcp.mjs, never by importing it. + "isRunnable", "tokenize", "isCacheableMcp", "redact", "cacheGet", + "cachePut", "cacheStats", "mcpCacheKey", + ]); + + const undocumented = []; + for (const f of readdirSync(join(ROOT, "lib")).filter((f) => f.endsWith(".mjs"))) { + const src = readFileSync(join(ROOT, "lib", f), "utf8"); + for (const m of src.matchAll(/^export (?:function|const) ([A-Za-z0-9_]+)/gm)) { + const name = m[1]; + if (INTERNAL.has(name)) continue; + if (!skill.includes(name)) undocumented.push(`${f}:${name}`); + } + } + + assert.deepEqual( + undocumented, + [], + `undocumented helper(s): ${undocumented.join(", ")}. Add them to the SKILL's ` + + `"API reference" section — an agent that can't find a signature there greps ` + + `lib/ at runtime, which cost 92 of 407 tool calls on one measured run.`, + ); +}); diff --git a/workflows/rca-batch.mjs b/workflows/rca-batch.mjs index f79e7a2..9d04a92 100644 --- a/workflows/rca-batch.mjs +++ b/workflows/rca-batch.mjs @@ -23,9 +23,25 @@ export const meta = { // { // csvPath, buildId, // manifest: { capability: { available, via } }, -// buildEvidence: { baselineRef, suspectWindow, ... }, // pre-computed once +// evidenceFilePath, // lib/evidence-file.mjs artifact for this build +// pluginRoot, // absolute path to this plugin, so coordinators can call bin/cached-exec.mjs +// buildEvidence: { baselineRef, isFallback, suspectWindow, reposCovered, workloadsCovered, gaps }, +// // ^ SHRUNK to a cheap summary/pointer only — the full PR list / log +// // sweeps live in the file at evidenceFilePath, read via each +// // coordinator's own Read tool. Repeating the full detail in every +// // dispatch prompt (as before) is exactly the duplication this file +// // removes. // clusters: [ -// { cluster_id, representative: { testRunId, testName, error_summary }, +// { cluster_id, +// representative: { testRunId, testName, error_summary, +// // Step 4b pre-dispatch outcome (SKILL.md Step 4b, lib/turn1-registry.mjs) +// // — at most one of these two is set, never both: +// turn1: { status: "PENDING", threadId, turnId } | +// { status: "NEEDS_INFO", threadId, asks }, +// // Step 4b's turn 1 already RESOLVED — no dispatch at all for this +// // representative; `resolved` is the RCA_SCHEMA-shaped result to use +// // directly (also already flipped into the CSV by the orchestrator). +// resolved: <RCA_SCHEMA object> | undefined }, // siblings: [ { testRunId, testName, error_summary } ] } // ] // } @@ -58,23 +74,83 @@ const clusters = ctx.clusters ?? []; const shared = [ `CSV state file: ${ctx.csvPath}`, `Capability manifest: ${JSON.stringify(ctx.manifest ?? {})}`, - `Build-level evidence (pre-computed once, reuse — do not re-fetch): ${JSON.stringify(ctx.buildEvidence ?? {})}`, + `Pre-fetched build-evidence file — READ THIS FIRST (via the Read tool) before making ANY live github/infra/logs gather call: ${ctx.evidenceFilePath}`, + `Build-evidence summary (full detail is in the file above; this is only a pointer — do not re-fetch what the file already covers): ${JSON.stringify(ctx.buildEvidence ?? {})}`, + `If the file's github/logs sections do not name a repo/workload/ask you need, or record a "gap" for it, that is a genuine gap — fall back to a live gather via the capability manifest above exactly as if no file existed. The file is an optimization, never a hard dependency.`, + `The file is read-write: after any live gather that fills a gap or goes deeper than what was there, write it back via contributeGithubEvidence/contributeLogsEvidence (lib/evidence-file.mjs) passing your own testRunId as writerId, before finishing this test — so a sibling dispatched after you, or another cluster sharing the same repo/workload, reads the enriched entry instead of re-fetching it. Each writer owns its own shard file, so concurrent coordinators cannot clobber each other; readers fold base + shards automatically.`, + `Tool cache — route read-only lookups through it so duplicate calls across coordinators become hits. Shell: node ${ctx.pluginRoot ?? "<pluginRoot>"}/bin/cached-exec.mjs <buildId> <yourTestRunId> '<gh|kubectl|curl|git command>' (behaves like the raw command; pipe to jq/grep OUTSIDE the wrapper so different filters share one fetch). MCP data queries: cached-mcp.mjs <buildId> get|put <tool> '<argsJson>' [writerId]. NEVER cache tfaRcaTurn/getTfaTurnResult/triggerRcaReport — they are stateful. Do not re-probe connectors the gate already validated.`, `Autonomous run — on an evidence gap with no valid connector, report "unavailable" back to TFA (NEVER prompt a user). Best-effort finalize.`, `PRODUCT_BUG / application-bug mandate: hunt the culprit PR via the github connector (deploy timeline vs last-pass window, changed paths vs failure signature) and feed the PR link(s) to TFA so related_prs populates. No PR after digging to the turn cap → state explicitly "no culprit PR identified after <what was searched>" so the CSV row records the gap.`, `Soft-PENDING is NOT an answer: tfaRcaTurn abandons its in-call poll at 90s while TFA keeps working. On status PENDING, call getTfaTurnResult(testRunId, turnId) FIRST and keep reading on the softPendingDrain budget (every 5s, <=40 reads / <=10min) until the status is RESOLVED / NEEDS_INFO / BLOCKED, then continue the loop. Reads do NOT count against the turn cap. Never submit a new message onto a turn still in flight. Only a fully spent drain budget ends the test PENDING.`, `Persist eagerly to the CSV: claim your row before turn 1, flip it on terminal (lib/csv-state.mjs).`, ].join("\n"); +function resumeLine(row) { + if (!row?.threadId || !row?.turnId) return null; + return [ + `RESUME (do not start a new thread): this test already has an in-flight thread`, + `threadId=${row.threadId} turnId=${row.turnId}.`, + `Call getTfaTurnResult(testRunId, turnId) FIRST to read its current state`, + `(drain any soft-PENDING per the softPendingDrain budget) before submitting`, + `anything further — reuse this threadId for every follow-up on this test.`, + row.last_evidence_digest ? `Prior evidence already gathered (reuse, don't re-fetch): ${row.last_evidence_digest}` : null, + row.root_cause ? `Prior attempt note: ${row.root_cause}` : null, + ].filter(Boolean).join("\n"); +} + +// Step 4b (SKILL.md Step 4b) already submitted this representative's turn 1, +// concurrently with Step 4's evidence pre-fetch. RESOLVED needs no coordinator +// dispatch at all (short-circuited in the pipeline stage below); these two +// non-terminal outcomes are handed to the coordinator instead of letting it +// submit turn 1 again — mutually exclusive per agents/ai-tfa-coordinator.md. +function turn1Line(r) { + const t = r?.turn1; + if (!t) return null; + if (t.status === "PENDING" && t.turnId) { + return [ + `RESUME (turn 1 already submitted by Step 4b — do not start a new thread):`, + `threadId=${t.threadId} turnId=${t.turnId}.`, + `Call getTfaTurnResult(testRunId, turnId) FIRST to read its current state`, + `(drain any soft-PENDING per the softPendingDrain budget) before submitting`, + `anything further — reuse this threadId for every follow-up on this test.`, + ].join("\n"); + } + if (t.status === "NEEDS_INFO") { + return [ + `TURN 1 ALREADY SUBMITTED AND ANSWERED by Step 4b — do NOT submit turn 1 again.`, + `threadId=${t.threadId}. turns_used starts at 1.`, + `TFA's turn-1 response was NEEDS_INFO with these asks (verbatim): ${JSON.stringify(t.asks ?? [])}`, + `Start this run at the ROUTE-the-asks step using them, then submit your first`, + `follow-up message on this SAME thread.`, + ].join("\n"); + } + return null; +} + function repPrompt(cluster) { const r = cluster.representative; + const resume = resumeLine(r); + // Mutual exclusivity, enforced in code, not just by convention: a + // representative gets AT MOST one resume-style instruction. A prior-run CSV + // pending-resume (`r.threadId`/`r.turnId`, an already in-flight thread from + // a run this build is resuming) takes precedence over a same-run Step 4b + // entry (`r.turn1`) — Step 4b's pre-dispatch is supposed to skip a + // representative already in pending-resume (SKILL.md Step 4b), but this is + // the backstop: presenting BOTH would hand the coordinator two different + // threadIds as "the" thread to resume, which is worse than picking one. + const t1 = resume ? null : turn1Line(r); return [ `You are the ai-tfa-coordinator for cluster ${cluster.cluster_id}.`, - `Run the FULL collaborative RCA loop for the representative test.`, + t1 + ? `Turn 1 was pre-dispatched by Step 4b — see below for how to resume it. Otherwise run the FULL collaborative RCA loop for the representative test.` + : `Run the FULL collaborative RCA loop for the representative test.`, `testRunId=${r.testRunId} testName=${r.testName ?? ""}`, `error_digest: ${r.error_summary ?? "(none)"}`, + resume, + t1, shared, `Return the structured RCA_OUTPUT for this test.`, - ].join("\n"); + ].filter(Boolean).join("\n"); } function siblingPrompt(sibling, repResult, cluster) { @@ -84,29 +160,40 @@ function siblingPrompt(sibling, repResult, cluster) { ` root_cause: ${repResult?.root_cause ?? "(representative did not resolve)"}`, ` related_prs: ${JSON.stringify(repResult?.related_prs ?? [])}`, `State this hypothesis on turn 1 and ask TFA to CONFIRM it against THIS test's own logs.`, + `The pre-fetched evidence file's data about your OWN workload is real evidence about YOUR OWN test — reading it is NOT blind inheritance. What must stay independent is the CONFIRMATION judgment: never adopt the representative's verdict just because the file already has the answer in it.`, `If TFA confirms in one turn → done. If it does NOT (NEEDS_INFO), fall back to the full loop — never blindly inherit.`, `testRunId=${sibling.testRunId} testName=${sibling.testName ?? ""}`, `error_digest: ${sibling.error_summary ?? "(none)"}`, + resumeLine(sibling), shared, `Return the structured RCA_OUTPUT for this test.`, - ].join("\n"); + ].filter(Boolean).join("\n"); } log(`Batch: ${clusters.length} cluster(s) over build ${ctx.buildId ?? "?"}`); // Pipeline: each cluster flows representative → siblings independently (no barrier // between stages), so a small cluster's siblings confirm while a big cluster's -// representative is still looping. Concurrency is bounded by the workflow runtime -// (~min(16, cores-2)); config.concurrency (5) is the intended soft target. +// representative is still looping. Concurrency is capped by the Workflow runtime +// at min(16, cores-2) — an architectural limit of the tool itself, not something +// this script or config.concurrency (20, see rca.config.json) can raise. That +// config value is an intended soft target/upper bound on THIS path only; the +// runtime queues anything beyond its own cap regardless of what this file says. const results = await pipeline( clusters, (cluster) => - agent(repPrompt(cluster), { - label: `rep:${cluster.representative.testRunId}`, - phase: "Representatives", - agentType: "tfa-rca:ai-tfa-coordinator", - schema: RCA_SCHEMA, - }).then((rca) => ({ cluster, rca })), + // Step 4b's turn 1 already RESOLVED this representative — no dispatch at + // all, zero added latency. The orchestrator already flipped this row's + // CSV entry to terminal; `resolved` just needs to flow into the sibling + // stage's pre_seed the same way a dispatched rep's result would. + cluster.representative?.resolved + ? Promise.resolve({ cluster, rca: cluster.representative.resolved }) + : agent(repPrompt(cluster), { + label: `rep:${cluster.representative.testRunId}`, + phase: "Representatives", + agentType: "tfa-rca:ai-tfa-coordinator", + schema: RCA_SCHEMA, + }).then((rca) => ({ cluster, rca })), ({ cluster, rca }) => parallel( (cluster.siblings ?? []).map((sib) => () =>