diff --git a/agents/ai-tfa-coordinator.md b/agents/ai-tfa-coordinator.md index f0284af..3ae257a 100644 --- a/agents/ai-tfa-coordinator.md +++ b/agents/ai-tfa-coordinator.md @@ -11,79 +11,50 @@ model: sonnet # Per-Test Collaborative RCA Coordinator (`ai-tfa-coordinator`) Drives the `tfaRcaTurn` MCP loop for a **single** failed test to a terminal RCA. -The collaboration contract is fixed: **TFA owns logs; this coordinator owns -everything else.** TFA (server-side, via the tool) reads the run's logs from its -own access and emits typed evidence asks; this coordinator fulfills every -**non-log** ask using whatever skills/tools the client has — routed through the -validated capability manifest — digests the findings, and feeds them back on the -same thread until TFA converges. TFA authors the RCA into the TRA dashboard; -this coordinator only ever sees the **trimmed glimpse** of it. The full report -lives on the Test Observability UI. - -This coordinator is **fully autonomous**: the `/rca-build` gate closed before it -was dispatched, so it **never prompts a user** — an evidence gap degrades to an -`unavailable` block back to TFA, always. - -This coordinator is the **reusable unit**: it takes one `testRunId` and runs -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*. +**TFA owns logs; this coordinator owns everything else.** Fulfills every non-log +ask via the validated capability manifest, digests findings, and feeds them back +until TFA converges. TFA authors the RCA; this coordinator sees only the +**trimmed glimpse**. The full report lives on the Test Observability UI. + +**Fully autonomous** — never prompts a user; evidence gaps degrade to +`unavailable`. **Generic over product and infra** — 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. +Invoke all independent tool calls simultaneously rather than sequentially. +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. + `/...` path in this file is relative to this value, not to + whatever directory you were started in. 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**. + states the hypothesis and asks TFA to confirm 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 + and landed `NEEDS_INFO`. When present, **do not submit turn 1** — start the + loop 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. + exclusive with `resume` and `pre_seed`: a representative gets at most one of + `resume`, `turn1_result`, or neither — never more than one, and never + alongside `pre_seed` (sibling-only). A Step 4b turn 1 that landed `RESOLVED` + needs no coordinator dispatch at all. - `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. + `github` and `logs`/`infra` evidence, keyed by repo/workload. Consult via + `evidence-show` before any live call (see Principle 0). Treat as read-WRITE: + live gathers that fill gaps are written back via + `contributeCodeEvidence`/`contributeLogsEvidence` (keyed by your + `testRunId`) so later dispatches benefit. 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 @@ -117,59 +88,36 @@ read-only and has no side effects, so a read is always safe to repeat. 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. + `Read`ing the path directly shows only the orchestrator's base file, + hiding per-writer shard contributions. 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)` + the one you need. `--prs` prints `mergedAt | #num | title` — anything + merged after the build started is disqualified without fetching a diff. + + Consult it before any live github/infra/logs call. Use what it covers + directly — entries are already digest-shaped; 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), or evidence + genuinely specific to this one test that a build-wide sweep could have + missed. For a sibling (`pre_seed` present): the file's data about YOUR OWN + test's workload/repo is real evidence, not inheritance — but the + CONFIRMATION judgment must still be independently yours (see principle 1). + + **Write back what you gather live.** Persist via + `contributeCodeEvidence(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. + `testRunId`**. Each coordinator writes only its own shard file under + `.contrib/`, so concurrent coordinators + never clobber each other; readers fold base + shards automatically. + Write back before finishing this test. Only write genuinely new/deeper + findings — never a no-op re-write. Best-effort: never block or retry. **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`: + shares digested findings; the cache shares raw call results. 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 @@ -181,45 +129,33 @@ read-only and has no side effects, so a read is always safe to repeat. 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: + pinned commit when available, otherwise falls through to the cached `gh` call: `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: + — a **branch name is refused** (local clones may be stale). + Check `localRepos` in the evidence file for which repos are local. + - **MCP data queries** (a log or metrics server, `listTestIds`, + `getFailureLogs`) — check first, 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. + (exit 0 = hit, skip the MCP call; exit 1 = miss, make the call then + `... put '' ` with the digest on stdin). + Skip caching for one-off queries only this test needs. - **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: + - Two wrapper gotchas: **(i)** hit/miss banners go to stderr — don't + `2>&1 | jq` (merges banner into pipe). **(ii)** commands containing single + quotes can't nest inside a single-quoted argument; pipe 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. + A pipe belongs outside the wrapper. **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 + list means "no PRs" ONLY when the entry also has `prsSearched: true`. + 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. + list — when untrusted, run the PR search live. Contribute the result back + (records `prsSearched`). 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. @@ -228,144 +164,76 @@ read-only and has no side effects, so a read is always safe to repeat. 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 — 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. +4. **One thread per test — with one narrow 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 context-exceeded restart in 4b. +4b. **`TFA agent run failed` — resubmit ONCE; if that also fails, RESTART.** + On the FIRST such failure, resubmit on the SAME thread (counts as a turn). + Do NOT mint a new thread or end `PENDING` on one failure alone. + + **Two consecutive same-thread failures** (no successful response between + them) indicate `context_length_exceeded` — the thread is structurally dead. + + 1. **Distill** everything gathered into ONE condensed hypothesis message + (leading root-cause, strongest evidence, suspect PR) in `pre_seed` + shape. Discard the dead thread's history. + 2. **Submit as turn 1 of a BRAND NEW thread** (`tfaRcaTurn(testRunId, + message=)`, no `threadId`). Capture the new + `threadId`; turns count against the same `turnCap`. + 3. **Allow exactly ONE restart per test.** If the fresh thread also hits + two consecutive failures, end `PENDING` (note `"likely-context-exceeded"`). 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. + - `TFA agent run failed` — the wedge; handle per 4b (resubmit once, then + condensed restart on two consecutive). + - `turn expired or not found` — a size rejection, NOT a thread/turn problem. + Shorten and resend before assuming the thread is broken. + - **`turnId` exists ONLY on a soft-`PENDING` turn.** `RESOLVED` / + `NEEDS_INFO` omit it — `turn_id: not available` is correct there. If you + end `pending-resume`, you MUST carry the `turnId` into `flip()` — the + resume path drains that exact turn before submitting anything new. 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, - observed case). So on `PENDING`, **call `getTfaTurnResult(testRunId, turnId)` - FIRST** and keep reading on the `softPendingDrain` budget - (`config/rca.config.json`: every 5s, ≤40 reads / ≤10min) until the status is - `RESOLVED` / `NEEDS_INFO` / `BLOCKED`. Only then route asks and submit the next - message. **Reads never count against the turn cap** — a drain re-reads the - *same* turn. Never submit a new message onto a turn still in flight: that - stacks two turns on one thread. Only when the drain budget is fully spent does - the run end `PENDING` (note `soft-pending`), resumable via `threadId`+`turnId`. - If the client has no `getTfaTurnResult` tool, end `PENDING` immediately as - before — never busy-wait through `tfaRcaTurn` resubmits instead. + truncated payload turns "grep found nothing" into a silent false negative. + 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` (5000)** — for + digest discipline (link, don't paste). Do not expect trimming to prevent + wedges; the wedge is a TFA-side fault. The reliable response is 4b + (resubmit), not shrinking the payload. + +5. **Soft-PENDING is DRAINED, not reported.** On `PENDING`, call + `getTfaTurnResult(testRunId, turnId)` and keep reading on the + `softPendingDrain` budget (`config/rca.config.json`: every 5s, ≤40 reads / + ≤10min) until status is `RESOLVED` / `NEEDS_INFO` / `BLOCKED`. Reads never + count against the turn cap. Never submit a new message onto a turn still in + flight. Only when the drain budget is fully spent does the run end `PENDING` + (note `soft-pending`), resumable via `threadId`+`turnId`. If the client has + no `getTfaTurnResult` tool, end `PENDING` immediately. 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 `/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. + before fulfilling any ask. The tool caps `message` at 5000 chars. 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. +9. **Field-filter every gather call, always.** Project down to only the + field(s) the ask needs — `--jq`, `-o custom-columns`, `-o jsonpath`, or a + piped `grep`/`head`. Never run the unfiltered form. This governs what + enters *your own* context (distinct from principle 6, which governs the + digest sent to TFA). Command templates: + `/skills/rca-build/references/github-evidence.md` § Field-filtering. ## Application bugs — the culprit-PR mandate (MANDATORY) -Whenever TFA's classification (in an ask, a suggestion, or the resolving -`glimpse.failure_type`) is **PRODUCT_BUG / application bug**, the github -connector is the deliverable, not optional evidence: +Whenever TFA's classification is **PRODUCT_BUG / application bug**, the github +connector is the deliverable: - **Hunt the culprit PR**: deploy timeline vs the last-pass window, changed paths vs the failure signature (`/skills/rca-build/references/github-evidence.md`), run the @@ -382,20 +250,13 @@ connector is the deliverable, not optional evidence: ## Suspect-PR falsification (github asks) 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 (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. +gather evidence via **GitHub MCP → `gh` → degrade**, and for each candidate +suspect **try to disprove it** (path overlap? shipped before failure window? +behind an OFF flag?). Feed both supporting and disconfirming evidence as a +structured suspect packet; only `verdict: supported` suspects belong in +`related_prs`. Reuse the `evidenceFile`'s `github` section when present and not +`gap`-marked; write deeper findings back via `contributeCodeEvidence`. Never +fabricate a PR when github is unavailable — emit an `unavailable` block. ## The loop @@ -423,24 +284,12 @@ capability is unavailable — emit an BLOCKED → END (PENDING, note "blocked") — terminal, no asks to route. NEEDS_INFO → go to 3. 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. + "high → medium → low" orders the ASSEMBLED MESSAGE only — gather calls + run CONCURRENTLY (parallel tool calls), not sequentially. `routeAsk`/ + `routeAsks` (`lib/routing.mjs`) classify each ask independently. Only + the final message assembly respects priority order. This applies within + a single ask too (e.g. multiple falsification probes for one github ask); + see `references/github-evidence.md` § "Batch every independent probe". For each ask: skip → record in asks_skipped, emit nothing. gather → FIRST check `evidenceFile` (if present) for this ask's scope — @@ -449,7 +298,7 @@ capability is unavailable — emit an 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`/ + result back via `contributeCodeEvidence`/ `contributeLogsEvidence` with your own testRunId as writerId (Operating Principle 0) so this fills the gap for whoever reads the file next. @@ -471,18 +320,13 @@ capability is unavailable — emit an 6. EMIT the RCA_OUTPUT block from the captured terminal state. ``` -> The loop mechanics above have an **executable mirror** in `lib/loop.mjs` -> (`runRcaLoop`) — conformance-tested against recorded `tfaRcaTurn` transcripts -> (`tests/conformance.test.mjs`). It also serves as the **sequential thin-client -> harness**: MCP clients without workflows/subagents drive the same contract -> by calling `runRcaLoop` with a real `submit` bound to `tfaRcaTurn`. +> Executable mirror: `lib/loop.mjs` (`runRcaLoop`), conformance-tested via +> `tests/conformance.test.mjs`. Also usable as a sequential thin-client harness. -**Sibling confirm (cluster member).** When `pre_seed` is present the first turn -states the representative's hypothesis and asks TFA to confirm against this -test's own logs. If TFA `RESOLVED`s in one turn → a logs-grounded per-test RCA at -minimal cost. If TFA instead returns `NEEDS_INFO` (the hypothesis does not hold -for this test), **fall back to the normal loop** — never blindly inherit the -representative's cause. +**Sibling confirm (cluster member).** When `pre_seed` is present, the first +turn states the representative's hypothesis for TFA to confirm against this +test's logs. If TFA returns `NEEDS_INFO`, **fall back to the normal loop** — +never blindly inherit the representative's cause. ## Output contract — `RCA_OUTPUT` @@ -538,11 +382,9 @@ RCA_OUTPUT_END ``` Notes: -- `status` is one of exactly three values. `turn-cap`, `soft-pending` (drain - 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. +- `status` is one of exactly three values. `turn-cap`, `soft-pending`, + `blocked`, and `likely-context-exceeded` all report as `PENDING`; note which + in `root_cause`. - `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 @@ -552,8 +394,7 @@ 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** treat a `gap`-marked `evidenceFile` entry as coverage (see P0). - **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. diff --git a/bin/cached-exec.mjs b/bin/cached-exec.mjs index b6ab9fd..8446c05 100644 --- a/bin/cached-exec.mjs +++ b/bin/cached-exec.mjs @@ -1,54 +1,30 @@ #!/usr/bin/env node -// Run a READ-ONLY command through the build's tool cache, in ONE tool call. +// Run a 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. +// Cached: IMMUTABLE reads (sha-pinned gh api, git show/cat-file/ls-tree/log with +// a sha) AND run-stable repo reads (gh pr view/diff/list, gh api repo reads, gh +// search, read-only git) — the latter don't change within a single minutes-long +// build RCA and are fetched identically by every sibling confirming the same +// suspect PRs. Live state (kubectl/curl/logs) passes through uncached. Mutations +// are refused. // -// Usage (command is ONE argument, so the caller's own quoting survives): +// Usage: // 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 { execSync } from "node:child_process"; +import { readFileSync } from "node:fs"; import { - toolCacheDirFor, cacheKey, cacheGet, cachePut, cacheStats, isRunnable, tokenize, + toolCacheDirFor, cacheKey, cacheGet, cachePut, cacheStats, + isCacheable, isImmutableRead, isRunStableRead, banner, } 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 = ""; - } + try { command = readFileSync(0, "utf8").trim(); } catch { command = ""; } if (!command) { console.error("[tool-cache] '-' given but stdin was empty"); process.exit(2); @@ -62,60 +38,34 @@ if (!buildId || (writerOrFlag !== "--stats" && !command)) { } 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)); + console.log(JSON.stringify({ cacheDir: dir, ...cacheStats(dir) }, 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}`); +// Refuse mutations. +if (!isCacheable(command)) { + console.error(`[tool-cache REFUSED] command looks mutating`); 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) { +// Run a command via the shell and return { stdout, exitCode }. +function run(cmd) { try { return { - stdout: execFileSync(argv[0], argv.slice(1), { + stdout: execSync(cmd, { encoding: "utf8", + shell: true, 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 }), + stdio: ["ignore", "pipe", "pipe"], }), exitCode: 0, }; } catch (err) { - if (err.stderr) process.stderr.write(err.stderr.toString()); // the only copy + if (err.stderr) process.stderr.write(err.stderr.toString()); return { stdout: (err.stdout ?? "").toString(), exitCode: typeof err.status === "number" ? err.status : 1, @@ -123,41 +73,38 @@ function run(argv, input) { } } -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]`); +const shouldCache = isImmutableRead(command) || isRunStableRead(command); +const key = cacheKey(command); + +// Try cache only for cacheable reads. +if (shouldCache) { + const hit = cacheGet(dir, key); + if (hit) { + banner(`[tool-cache HIT ${key} — captured by ${hit.writerId ?? "?"}, ${hit.bytes}B]`, logPath); + process.stdout.write(hit.stdout); + process.exit(0); } } -// 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; } +// Execute the command (cached or pass-through). +const res = run(command); + +if (res.exitCode !== 0) { + banner(`[tool-cache MISS ${key} — exited ${res.exitCode}, NOT cached]`, logPath); + process.stdout.write(res.stdout); + process.exit(res.exitCode); +} + +if (shouldCache) { + if (res.stdout.trim() === "") { + banner(`[tool-cache MISS ${key} — empty result, NOT cached]`, logPath); + } else { + cachePut(dir, key, { command, writerId: writerOrFlag, stdout: res.stdout, exitCode: 0 }, Date.now()); + banner(`[tool-cache MISS ${key} — stored ${res.stdout.length}B]`, logPath); + } +} else { + banner(`[tool-cache PASS-THROUGH — not a cacheable read]`, logPath); } -process.stdout.write(out); -process.exit(finalExit); +process.stdout.write(res.stdout); +process.exit(0); diff --git a/bin/cached-mcp.mjs b/bin/cached-mcp.mjs index a5074ee..72760d4 100644 --- a/bin/cached-mcp.mjs +++ b/bin/cached-mcp.mjs @@ -2,53 +2,28 @@ // 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: -// +// Usage: // 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. +// 2. put → node bin/cached-mcp.mjs put '' # payload on stdin +// 3. list → node bin/cached-mcp.mjs list +// 4. stats → node bin/cached-mcp.mjs stats // // 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. +// `triggerRcaReport`. Those are stateful. -import { readFileSync, readdirSync, existsSync, appendFileSync } from "node:fs"; +import { readFileSync, readdirSync, existsSync } from "node:fs"; import { join } from "node:path"; import { - toolCacheDirFor, mcpCacheKey, cacheGet, cachePut, cacheStats, isCacheableMcp, + toolCacheDirFor, mcpCacheKey, cacheGet, cachePut, cacheStats, isCacheableMcp, banner, } 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 list"); console.error(" cached-mcp.mjs stats"); process.exit(2); } @@ -60,25 +35,19 @@ if (verb === "stats") { 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 + if (!/^mcp__/.test(e.command ?? "")) continue; 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)"); + if (!n) console.log("(no MCP entries cached)"); process.exit(0); } @@ -93,9 +62,7 @@ if (!isCacheableMcp(tool)) { } let args; -try { - args = JSON.parse(argsJson); -} catch (err) { +try { args = JSON.parse(argsJson); } catch (err) { console.error(`[mcp-cache] argsJson is not valid JSON: ${err.message}`); process.exit(2); } @@ -105,27 +72,23 @@ 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`); + banner(`[mcp-cache MISS ${key} ${tool}] — make the MCP call, then 'put' the digest`, logPath); process.exit(1); } - banner(`[mcp-cache HIT ${key} ${tool} — captured by ${hit.writerId ?? "?"}, ${hit.bytes}B]`); + banner(`[mcp-cache HIT ${key} ${tool} — captured by ${hit.writerId ?? "?"}, ${hit.bytes}B]`, logPath); process.stdout.write(hit.stdout); process.exit(0); } if (verb === "put") { let payload = ""; - try { - payload = readFileSync(0, "utf8"); // stdin - } catch { - payload = ""; - } + try { payload = readFileSync(0, "utf8"); } 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]`); + banner(`[mcp-cache STORED ${key} ${tool} — ${rec.bytes}B]`, logPath); process.exit(0); } diff --git a/bin/prefetch-prs.mjs b/bin/prefetch-prs.mjs new file mode 100644 index 0000000..6e1e457 --- /dev/null +++ b/bin/prefetch-prs.mjs @@ -0,0 +1,87 @@ +#!/usr/bin/env node +// Deterministically pre-fetch a repo's merged-PR window into the build evidence +// file, in the CANONICAL shape, in ONE call — so the orchestrator never hand-rolls +// the entry (the failure mode: a `{deployState, prCount5d, topPRs}` blob that +// readers ignore because they only read `prsInWindow`, defeating the whole +// pre-fetch and forcing every coordinator to re-run `gh pr list` live). +// +// node bin/prefetch-prs.mjs +// +// Runs the FIRST PR-list call WITH `files` (per SKILL.md Step 4), writes +// `prsInWindow: [{pr, title, mergedAt, url, files:[…]}]` + `prsSearched: true` +// via setCodeEvidence — merging, so an existing `deployState` is preserved. +// Emits a one-line summary. Uses the GitHub CLI (`gh`); a different GitHub +// capability should pre-fetch through its own connector and write the same shape. + +import { execFileSync } from "node:child_process"; +import { + evidencePathFor, setCodeEvidence, readBaseFile, +} from "../lib/evidence-file.mjs"; + +/** Pure: map `gh pr list --json …,files` output to canonical prsInWindow rows. + * Exported for tests — no I/O, no network. */ +export function normalizePrs(raw) { + const list = Array.isArray(raw) ? raw : []; + return list.map((pr) => ({ + pr: pr.number ?? pr.pr ?? null, + title: pr.title ?? "", + mergedAt: pr.mergedAt ?? null, + url: pr.url ?? null, + files: Array.isArray(pr.files) + ? pr.files.map((f) => (typeof f === "string" ? f : f?.path)).filter(Boolean) + : [], + })); +} + +// --- CLI --- +const isMain = import.meta.url === `file://${process.argv[1]}`; +if (isMain) { + const [, , buildId, repo, branch, from, to] = process.argv; + if (!buildId || !repo || !branch || !from || !to) { + console.error("usage: prefetch-prs.mjs "); + process.exit(2); + } + + let raw; + try { + const out = execFileSync( + "gh", + [ + "pr", "list", "-R", repo, "--state", "merged", "--base", branch, + "--search", `merged:${from}..${to}`, + "--json", "number,title,mergedAt,url,files", + "--limit", "100", + ], + { encoding: "utf8", maxBuffer: 64 * 1024 * 1024 }, + ); + raw = JSON.parse(out || "[]"); + } catch (err) { + // A failed search is a genuine gap, never a blocker — record it so readers + // know the list was ATTEMPTED (not silently empty) and can fall back to live. + const path = evidencePathFor(buildId, process.env.RCA_STATE_DIR ?? ""); + const base = readBaseFile(path); + const prev = (base.github ?? {})[repo] ?? {}; + setCodeEvidence(path, repo, { + deployState: prev.deployState ?? null, + prsInWindow: [], + prsSearched: false, + gap: `pr-list search failed: ${String(err.message || err).slice(0, 120)}`, + }, Date.now()); + console.error(`[prefetch-prs] ${repo}: search FAILED — recorded gap, readers will fall back to live`); + process.exit(1); + } + + const prsInWindow = normalizePrs(raw); + const path = evidencePathFor(buildId, process.env.RCA_STATE_DIR ?? ""); + const base = readBaseFile(path); + const prev = (base.github ?? {})[repo] ?? {}; + setCodeEvidence(path, repo, { + deployState: prev.deployState ?? null, // preserve an already-fetched deployState + prsInWindow, + prsSearched: true, + gap: null, + }, Date.now()); + + const withFiles = prsInWindow.filter((p) => p.files.length > 0).length; + console.log(`[prefetch-prs] ${repo}: ${prsInWindow.length} PR(s) in window, ${withFiles} with files → prsInWindow`); +} diff --git a/config/rca.config.json b/config/rca.config.json index aea7ace..8292879 100644 --- a/config/rca.config.json +++ b/config/rca.config.json @@ -1,10 +1,10 @@ { "$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", - "$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.", + "$concurrencyComment": "Advisory fan-out width for Step 5. Honored on the default direct-dispatch path — the orchestrator batches this many ai-tfa-coordinator subagents in one message (the host may run slightly fewer at once). The sequential harness (lib/loop.mjs) runs one test at a time and ignores this; the opt-in Workflow path (workflows/rca-batch.mjs) is capped by its own runtime. See SKILL.md Step 5.", "concurrency": 20, "turnCap": 6, - "turnMessageMaxChars": 1000, + "turnMessageMaxChars": 5000, "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.", diff --git a/lib/build-cleanup.mjs b/lib/build-cleanup.mjs deleted file mode 100644 index e8faab7..0000000 --- a/lib/build-cleanup.mjs +++ /dev/null @@ -1,59 +0,0 @@ -// 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 5d636f5..39a982b 100644 --- a/lib/csv-state.mjs +++ b/lib/csv-state.mjs @@ -342,23 +342,6 @@ export function flip(csvPath, testRunId, fields, nowMs) { // 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 — ` + diff --git a/lib/evidence-file.mjs b/lib/evidence-file.mjs index e7e42d5..9af4ea3 100644 --- a/lib/evidence-file.mjs +++ b/lib/evidence-file.mjs @@ -325,13 +325,47 @@ export function setBaseline(filePath, baseline, suspectWindow, nowMs) { return doc; } +/** Canonical top-level keys of a `doc.github[repo]` entry. Everything else is + * a caller mistake (see `assertGithubEntry`). */ +const GITHUB_ENTRY_KEYS = new Set(["deployState", "prsInWindow", "prsSearched", "gap"]); + +/** Guard the code-evidence entry shape at the write boundary. + * + * `setCodeEvidence` stores the entry VERBATIM, so a caller that hand-rolls a + * shape — e.g. `{ deployState, prCount5d, topPRs }` instead of the canonical + * `{ deployState, prsInWindow, … }` — silently produces a file whose PR list + * every reader (`evidence-show`, `hasTrustworthyPrList`, the coordinators) + * treats as "never searched", because they only read `prsInWindow`. The whole + * build-level pre-fetch is then defeated with no error, and every coordinator + * re-fetches the PR list live. Fail loud here instead of shipping a dead file. */ +export function assertGithubEntry(entry, repo = "?") { + if (entry === null || typeof entry !== "object" || Array.isArray(entry)) { + const got = entry === null ? "null" : Array.isArray(entry) ? "array" : typeof entry; + throw new TypeError(`github entry for '${repo}' must be an object, got ${got}`); + } + const unknown = Object.keys(entry).filter((k) => !GITHUB_ENTRY_KEYS.has(k)); + if (unknown.length) { + throw new Error( + `github entry for '${repo}' has unknown key(s) [${unknown.join(", ")}]. ` + + `Canonical shape: { deployState, prsInWindow: [{pr, files, …}], prsSearched, gap }. ` + + `A merged-PR list MUST be stored as 'prsInWindow' with each PR's 'files' — readers ` + + `ignore any other key, so a mis-shaped entry silently reads as "never searched".`, + ); + } + if (entry.prsInWindow !== undefined && !Array.isArray(entry.prsInWindow)) { + throw new TypeError(`github entry for '${repo}': prsInWindow must be an array`); + } + return entry; +} + /** 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) { + * in the file is untouched. The entry shape is validated (`assertGithubEntry`). */ +export function setCodeEvidence(filePath, repo, entry, nowMs) { + assertGithubEntry(entry, repo); const doc = loadOrInit(filePath, nowMs); doc.github[repo] = entry; doc.generatedAtMs = nowMs; @@ -341,7 +375,7 @@ export function setGithubEvidence(filePath, repo, entry, nowMs) { /** 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 + * Same no-clobber guarantee as `setCodeEvidence`, keyed by workload instead * of repo. */ export function setLogsEvidence(filePath, workload, entry, nowMs) { const doc = loadOrInit(filePath, nowMs); @@ -387,7 +421,8 @@ function writeShard(path, doc) { * 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) { +export function contributeCodeEvidence(basePath, writerId, repo, patch, nowMs) { + assertGithubEntry(patch, repo); 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; @@ -408,7 +443,7 @@ export function contributeGithubEvidence(basePath, writerId, repo, patch, nowMs) } /** Contribute what THIS coordinator gathered live for a workload's app-logs. - * Same single-writer-shard discipline as `contributeGithubEvidence`; + * Same single-writer-shard discipline as `contributeCodeEvidence`; * `clusterIds` is unioned rather than replaced. */ export function contributeLogsEvidence(basePath, writerId, workload, patch, nowMs) { const { path, doc } = loadOwnShard(basePath, writerId, nowMs); diff --git a/lib/glimpse.mjs b/lib/glimpse.mjs deleted file mode 100644 index 38d7fb0..0000000 --- a/lib/glimpse.mjs +++ /dev/null @@ -1,43 +0,0 @@ -// Terse end-of-run summary — the ONLY in-client output of a /rca-build run. -// Deliberately a COMPLETION NOTICE, not a report: status counts + the UI link, -// nothing else. Root causes, culprit PRs, per-test analysis, cluster breakdowns -// live ONLY on the Test Observability dashboard (triggerRcaReport → viewReport). -// Do not reintroduce per-test cause/PR lines here — that is the whole point of -// the trim. Degrade, don't crash: missing fields are treated as absent. - -import { readRows } from "./csv-state.mjs"; - -// Map raw CSV rca_done states → the three buckets a human cares about. -function bucket(state) { - const s = (state || "").toLowerCase(); - if (s === "resolved") return "resolved"; - if (s === "pending" || s === "pending-resume") return "pending"; - return "failed"; -} - -// Render the completion summary. Returns a plain-text block: -// RCA analysis complete — build -// tests · resolved ·

pending · failed -// (the caller appends the "Full report on the Test Observability UI: " -// line from triggerRcaReport's viewReport — see SKILL.md Step 6). -export function renderGlimpse(rows, { buildId } = {}) { - const head = `RCA analysis complete${buildId ? ` — build ${buildId}` : ""}`; - if (!rows || rows.length === 0) { - return `${head}\nNo failed tests analyzed.\n`; - } - const counts = rows.reduce( - (acc, r) => { - acc[bucket(r.rca_done)] += 1; - return acc; - }, - { resolved: 0, pending: 0, failed: 0 }, - ); - const parts = [`${rows.length} test(s)`, `${counts.resolved} resolved`]; - if (counts.pending) parts.push(`${counts.pending} pending`); - if (counts.failed) parts.push(`${counts.failed} failed`); - return `${head}\n${parts.join(" · ")}\n`; -} - -export function renderGlimpseFromCsv(csvPath, opts = {}) { - return renderGlimpse(readRows(csvPath), opts); -} diff --git a/lib/signature.mjs b/lib/signature.mjs index 0705835..e4c7a8b 100644 --- a/lib/signature.mjs +++ b/lib/signature.mjs @@ -1,45 +1,11 @@ -// Failure-signature clustering (ideation #1). A red build's N failures usually -// trace to a handful of causes; clustering collapses the expensive evidence hunt -// to O(distinct causes). The signature is computed from the trimmed failure -// detail U1 surfaces on each listTestIds row (category + first error line + file -// path) — no extra probe turns. +// Cluster helpers: pick a stable representative for a server-computed failure +// theme, and build the pre-seed a sibling needs from its representative's +// already-landed CSV row. Dependency-free + deterministic (no crypto, no clock, +// no random) so it is usable from the workflow sandbox and trivially testable. // -// Dependency-free + deterministic (no crypto, no clock, no random) so it is -// usable from the auto-mode workflow sandbox and trivially testable. - -// Normalize a string for signature comparison: lowercase and fold the volatile -// tokens that make two instances of the SAME failure look different (ids, -// timestamps, hex/uuids, line:col, bare numbers). -export function normalize(value) { - return String(value ?? "") - .toLowerCase() - .replace(/\b\d{4}-\d{2}-\d{2}[t ]\d{2}:\d{2}:\d{2}\S*/g, "") // ISO timestamps - .replace(/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/g, "") - .replace(/0x[0-9a-f]+/g, "") // memory addresses - .replace(/:\d+(:\d+)?\b/g, ":") // file:line(:col) - .replace(/\d+/g, "") // remaining numbers (incl. unit-suffixed, e.g. 3000ms) - .replace(/\s+/g, " ") - .trim(); -} - -// The signature triple: normalized category | error summary | file path. -export function computeSignature(row) { - const category = normalize(row.failure_category); - const error = normalize(row.error_summary); - const file = normalize(row.file_path); - const sig = `${category}|${error}|${file}`; - return sig.replace(/\|/g, "").trim().length === 0 ? "" : sig; -} - -// Deterministic short id for a signature string (FNV-1a → base36). -function hashId(s) { - let h = 0x811c9dc5; - for (let i = 0; i < s.length; i++) { - h ^= s.charCodeAt(i); - h = Math.imul(h, 0x01000193); - } - return (h >>> 0).toString(36); -} +// Clustering comes from the server (lib/theme-clustering.mjs + +// getBuildFailureThemes); when the server returns no themes, every failed test +// is its own representative (a singleton). // A stable representative for a cluster: prefer a non-flaky member (a flaky test // is a poor exemplar), then the smallest testRunId. Deterministic. @@ -52,61 +18,6 @@ export function selectRepresentative(members) { })[0]; } -// Cluster rows by signature. Mutates each row's `cluster_id`. Rows with no -// signal (empty signature) become their own singleton (never merged into a -// catch-all). Returns { rows, clusters } where each cluster carries its -// representative + siblings. -export function clusterRows(rows) { - const groups = new Map(); - - for (const row of rows) { - const sig = computeSignature(row); - const id = sig === "" ? `solo-${row.testRunId}` : `c-${hashId(sig)}`; - row.cluster_id = id; - if (!groups.has(id)) groups.set(id, { cluster_id: id, signature: sig, members: [] }); - groups.get(id).members.push(row); - } - - const clusters = []; - for (const group of groups.values()) { - const representative = selectRepresentative(group.members); - const siblings = group.members.filter((m) => m !== representative); - clusters.push({ ...group, representative, siblings }); - } - - 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 diff --git a/lib/state-dir.mjs b/lib/state-dir.mjs index d91570e..9fa7ed2 100644 --- a/lib/state-dir.mjs +++ b/lib/state-dir.mjs @@ -2,17 +2,12 @@ // // 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). +// plugin. That is deliberate: this is the user's machine, resume is keyed on +// buildId → same path, and reclaiming the OS temp dir is the OS's job, not +// ours. `hardenStateDir` only tightens permissions (owner-only) — it never +// deletes — and is cheap enough to run unconditionally at gate startup. -import { existsSync, readdirSync, statSync, chmodSync, rmSync } from "node:fs"; +import { existsSync, readdirSync, statSync, chmodSync } from "node:fs"; import { join } from "node:path"; /** @@ -72,65 +67,3 @@ export function hardenStateDir(dir) { 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 index 3826ef8..2619d24 100644 --- a/lib/theme-clustering.mjs +++ b/lib/theme-clustering.mjs @@ -1,13 +1,12 @@ -// 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`. +// Server-computed failure-theme clustering (`buildThemes`/`flat` via the +// getBuildFailureThemes/listTestsInFailureTheme MCP tools) — the clustering +// path (skills/rca-build/SKILL.md Step 3). When the server returns no themes, +// pass an empty `buildThemes` and every failed test falls through to its own +// singleton cluster (i.e. all tests become representatives). // -// 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. +// Pure + dependency-free: takes already-fetched plain data in, returns +// { rows, clusters }, so downstream code (the fan-out workflow, the sequential +// harness) is agnostic to how many themes the server produced. import { selectRepresentative } from "./signature.mjs"; @@ -17,9 +16,8 @@ import { selectRepresentative } from "./signature.mjs"; // 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()`. +// assign to a theme: never silently dropped, it becomes its own singleton. +// Mutates each row's `cluster_id` (the caller persists via writeRows). // // 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 diff --git a/lib/tool-cache.mjs b/lib/tool-cache.mjs index 268e78f..fff88c0 100644 --- a/lib/tool-cache.mjs +++ b/lib/tool-cache.mjs @@ -1,34 +1,15 @@ -// Build-scoped memo cache for READ-ONLY tool calls (`gh`, `kubectl`, curl, …). +// Build-scoped memo cache for READ-ONLY tool calls. // -// 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. +// Only IMMUTABLE reads are cached: gh api calls pinned to a sha or addressing +// git objects (/git/ path), and git commands that reference a 40-hex sha +// (git show, git cat-file, git ls-tree, git log). Everything else passes +// through uncached — the wrapper still runs it and returns its real output. // // 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, @@ -36,262 +17,104 @@ import { import { join } from "node:path"; import { tmpdir } from "node:os"; import { createHash } from "node:crypto"; +import { appendFileSync } from "node:fs"; -/** Per-build cache directory, sitting alongside the state CSV and evidence - * file under the same OS-temp convention. */ +/** Per-build cache directory. */ 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 */ } } +/** Stable key for one call. Whitespace is normalized. */ 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. +// Commands that must never run through the wrapper at all. 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; -} - +/** True if the command is NOT a known mutation. */ 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", -]); +// 40-hex-char SHA pattern +const SHA40 = /\b[0-9a-f]{40}\b/i; -/** 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. + * True if the command's output is provably immutable and should be memoized. * - * 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. + * Cacheable commands: + * - `gh api` with `?ref=<40-hex-sha>` OR a `/git/` path (blobs/trees/commits) + * - `git show :...`, `git cat-file ... `, `git ls-tree `, + * `git log ` — i.e. a 40-hex sha present in the command */ -export function isRunnable(command) { +export function isImmutableRead(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" }; + // gh api calls pinned to immutable git objects + if (/^\s*gh\s+api\s/i.test(c)) { + // Contains /git/ path (blobs, trees, commits) + if (/\/git\//.test(c)) return true; + // Contains ?ref=<40-hex-sha> or &ref=<40-hex-sha> + if (/[?&]ref=[0-9a-f]{40}\b/i.test(c)) return true; + return false; } - 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(", ")})`, - }; - } + // git commands with a 40-hex sha present + if (/^\s*git\s+(show|cat-file|ls-tree|log)\s/i.test(c) && SHA40.test(c)) { + return true; } - return { ok: true, fetch: parsed[0], filters: parsed.slice(1), fetchText: segments[0] }; + return false; } /** - * 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. + * True if the command is a repo read that is stable for the lifetime of ONE + * build-RCA run (minutes) though not provably immutable. These are the reads the + * cross-coordinator cache exists to collapse: a build's suspect PRs don't change + * mid-run, and every sibling confirms the SAME representative's suspect PRs — so + * `gh pr view/diff `, repo content reads, and read-only git are fetched + * identically by many coordinators. Caching them per-build removes that N-fold + * duplication. + * + * Deliberately NOT included: `kubectl get/logs`, `curl`, `aws`, `docker`, log + * queries — live state that changes second-to-second and must always pass + * through. Mutations are refused upstream by `isCacheable`. */ -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; +export function isRunStableRead(command) { + const c = String(command ?? ""); + // read-only gh PR/repo subcommands (by number or path) + if (/^\s*gh\s+pr\s+(view|diff|list|checks|status)\b/i.test(c)) return true; + if (/^\s*gh\s+search\s+(code|prs|issues|commits|repos)\b/i.test(c)) return true; + // gh api GET on repo/pull/content/commit/compare paths (writes already refused) + if ( + /^\s*gh\s+api\s/i.test(c) && + !/(^|\s)(-X|--method)\b/i.test(c) && + /\brepos\/[^\s]+\/(contents|pulls|commits|compare|git)\b/i.test(c) + ) { + return true; } - if (quote) throw new Error("unterminated quote in command"); - if (started) out.push(cur); - return out; + // read-only git history/blob inspection (sha optional — run-stable) + if (/^\s*git\s+(show|log|diff|cat-file|ls-tree|blame|rev-parse)\b/i.test(c)) return true; + return false; } // ---- 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); @@ -304,27 +127,11 @@ export function mcpCacheKey(toolName, 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. +// Redact secrets before persisting. 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}`) @@ -332,7 +139,6 @@ export function redact(text) { } const MAX_BYTES = 256 * 1024; - let tmpSeq = 0; export function cacheGet(cacheDir, key) { @@ -341,25 +147,11 @@ export function cacheGet(cacheDir, key) { try { return JSON.parse(readFileSync(p, "utf8")); } catch { - return null; // half-written or corrupt -> treat as a miss, never throw + return null; } } -/** 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; @@ -374,16 +166,12 @@ export function cachePut(cacheDir, key, entry, nowMs) { 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 + renameSync(tmpPath, finalPath); 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; @@ -393,9 +181,18 @@ export function cacheStats(cacheDir) { entries++; try { bytes += JSON.parse(readFileSync(join(cacheDir, f), "utf8")).bytes ?? 0; - } catch { - /* skip */ - } + } catch { /* skip */ } } return { entries, bytes }; } + +// ---- Shared banner utility ------------------------------------------------ + +export function banner(line, logPath) { + console.error(line); + if (logPath) { + try { + appendFileSync(logPath, line + "\n", { encoding: "utf8", mode: 0o600 }); + } catch { /* logging must never break the fetch */ } + } +} diff --git a/lib/turn1-registry.mjs b/lib/turn1-registry.mjs index d0674c9..f4540b8 100644 --- a/lib/turn1-registry.mjs +++ b/lib/turn1-registry.mjs @@ -23,7 +23,7 @@ // 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 { readFileSync, writeFileSync, existsSync, mkdirSync, chmodSync } from "node:fs"; import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; @@ -107,11 +107,3 @@ 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 b62e415..68c6200 100644 --- a/skills/rca-build/SKILL.md +++ b/skills/rca-build/SKILL.md @@ -27,116 +27,17 @@ 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. +Fan out independent work in one message — connector probes, per-repo +evidence fetches, per-workload log sweeps. Only chain calls when one's +output is a literal input to the next. -## API reference — read THIS, do not grep the source +## API reference — read `references/api.md`, don't 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. +The `lib/`+`bin/` signatures the coordinator calls live in +[`references/api.md`](references/api.md). Load it the first time you need a +signature (Step 2 onward) — not at gate time. Grepping `lib/` at runtime to +relearn the API is the drift this file exists to prevent. ## Step 0 — input @@ -158,104 +59,22 @@ pass. The gate has two parts; both run before any RCA work starts. ### Part A — connector discovery + validation -**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: +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, ECS, docker, Nomad, plain VMs, PM2, …), **logs** (kibana or any log store), **metrics**, **other**; - plus any connector-shaped skills / MCP servers present in the session - (a log-search MCP, a metrics MCP, an infra skill, …). + (a log-search MCP, a metrics MCP, an infra skill, …). A skill that declares + `capability: github | infra | logs | metrics | other` **supersedes** the raw + tool for that capability — it carries product-specific routing (repo map, + branch conventions) the raw tool lacks. **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: +being absent) never blocks another connector's probe from running. - **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 @@ -263,12 +82,7 @@ that, so treat it as a hard gate, not a preference: 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." + that call's result — STOP.** Add the rest of the list to it before sending. - 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. @@ -289,7 +103,7 @@ 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 +1. For every connector skill added to the manifest in Part A, 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.** @@ -311,12 +125,8 @@ 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. -**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 +**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 @@ -356,17 +166,10 @@ is the point: **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 +these fields are answerable outright for its product. 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. +doesn't match the failure signature at all), don't force its default — treat +the field via the normal path (inference, else a gap) 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 @@ -401,17 +204,12 @@ 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. -**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. +**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. There is no second gate question, ever. **Headless: skip asking entirely; record the gaps.** @@ -437,18 +235,8 @@ listTestIds(buildId=, status="failed", includeFailureDetail=true) 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. +The sweep is cheap and idempotent — run it unconditionally; it never throws, +skipping anything it cannot chmod. Resolve the state file with `lib/csv-state.mjs` → `csvPathFor(buildId, config.paths.stateDir)` — the **build id is in the filename** and the default @@ -464,94 +252,13 @@ write an empty CSV, report "no failed tests", stop. ## Step 3 — clustering (see `/skills/rca-build/references/clustering.md`) -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. - -**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. - -1. Call `getBuildFailureThemes(buildUuid=)`. If nothing has ever - been computed for this build, this triggers computation (one POST, same - call) and polls in-call for `buildThemeWorkflow.status` to reach `SUCCESS`. - The poll cadence is fixed: **one GET first; a single POST trigger only when - the build has no themes yet (never re-fired); then GET every 3s up to a 90s - wall-clock ceiling.** Reaching `SUCCESS` returns `ready: true`; exhausting - the 90s (or a `FAILED`/`ERROR` status) returns `ready: false`. The call - never blocks longer than ~90s — so it's safe to await inline. -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. - - **`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`** — a **server-outage net, not the routine path.** The - trigger endpoint is deployed, so a never-computed build gets its themes from - the POST inside Step 1 and returns `ready: true`; `ready: false` now means - the server genuinely couldn't produce them (still computing past the poll - budget, a failure status, or `status: "trigger-unavailable"` — the trigger - call itself errored). Only then **fall back** to - **`clusterAndPersist(csvPath, csvStateModule)`** (`lib/signature.mjs`), not - `clusterRows` directly: +Cluster from the server's failure themes so each *cause* runs one +**representative** (full loop) + `N−1` **siblings** (one-turn confirm) while every +test still lands a per-test RCA; no themes → every test its own singleton. - ```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. With the trigger - endpoint deployed this path is the exception, not the rule: a healthy server - reaches `ready: true` on its own (fresh builds included), and this net only - engages on a genuine outage — the server erroring, failing computation, or a - real backlog outrunning the poll budget. - -`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). +**Run the call sequence and invariants in `references/clustering.md` (§ Running +it).** After it, `writeRows(csvPath, rows)` and verify: if any row's `cluster_id` +is empty, Step 3 did not take effect — do not proceed. ## Step 4 — build-evidence pre-fetch (see `/skills/rca-build/references/evidence-routing.md` and `/lib/evidence-file.mjs`) @@ -562,13 +269,9 @@ 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. +starts the moment Step 3 finishes and runs the whole time Step 4 does — any +progress line should say `Evidence pre-fetch (Step 4) + turn-1 pre-dispatch +(Step 4b)`, never "Step 4 done, now starting Step 4b." 1. Resolve the evidence-file path: `lib/evidence-file.mjs` → `evidencePathFor(buildId, config.paths.stateDir)` — @@ -576,50 +279,37 @@ leaves the other free to reintroduce the bug. `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`). + `repos_validated` list (a build's failures often span several repos — + validate the full set the connector maps, not one guessed repo). - **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)`. + Persist the deploy-state via + `setCodeEvidence(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. + **For the PR window, do NOT hand-build the entry — use the deterministic + helper**, once per repo, all repos fired as parallel tool calls in ONE message: ```bash - gh pr list -R / --state merged --base \ - --search 'merged:..' --json number,title,mergedAt,url,files --limit 100 + node bin/prefetch-prs.mjs ``` - `--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 + It runs the `--json number,title,mergedAt,url,files --limit 100` search and + writes the **canonical `prsInWindow` (with `files`) + `prsSearched: true`** via + `setCodeEvidence`, preserving any `deployState` already recorded. **Never author + the github entry by hand** (e.g. a `{prCount5d, topPRs}` blob): readers consume + only `prsInWindow`, so a mis-shaped entry silently reads as "never searched" and + every coordinator re-fetches the list live. `setCodeEvidence` now **rejects** + non-canonical keys (`assertGithubEntry`) so this fails loud instead of shipping a + dead file. A non-`gh` GitHub capability pre-fetches through its own connector but + writes the identical shape. + + Why `files` matters: 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 @@ -630,30 +320,18 @@ evidenceType, fn)` to dedupe if two steps need the same `(repo, range)`. 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 + - **Never let coordinators re-probe connectors.** State plainly in the + dispatch prompt that the gate validated them. + - **Do NOT bulk-fetch file contents.** 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 + "now". **Batch all workload sweeps together with repo fetches from step 3.** + **PAD the window: `started_at − 2m` .. `finished_at + 10m`.** 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, + coordinator can weigh it; do NOT silently widen to an arbitrary window. + Persist via `setLogsEvidence(path, workload, {clusterIds, kubectlSweep, victorialogs, gap}, nowMs)`. Two query mechanics that cost real calls when missed: @@ -672,11 +350,8 @@ evidenceType, fn)` to dedupe if two steps need the same `(repo, range)`. 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. +6. **Resolve local clones ONCE** (`lib/repo-source.mjs`). Local `git show` + serves the same bytes as `gh api` with no network round trip. ```js const d = discoverWorkspaceRoot({ repos: reposValidated, from: pluginRoot }); @@ -688,32 +363,24 @@ evidenceType, fn)` to dedupe if two steps need the same `(repo, range)`. ``` `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. + candidate directory only if it actually contains one of *this run's* repos, + bounded to ~3 tries. Finding nothing is fine: 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. + `deployShas()` falls back to parsing prose `summary`, but that is a + safety net, not the contract. `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. + branch names — a local branch may be stale. - 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`, +`block` per `evidence-routing.md`'s caps (`SUMMARY≤400`, `SNIPPET≤20/40 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. @@ -723,27 +390,19 @@ 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. +finishes. Turn 1's message has no dependency on Step 4's evidence pre-fetch — +it is built entirely from Step 2's CSV seed (`error_summary`/`testName`). So +there is no need to wait for Step 4 before starting Step 4b, or to wait for +Step 4b 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. +drain. Write a minimal, purpose-built inline prompt (not a full +`ai-tfa-coordinator` dispatch), and put the exact output contract below +directly in that prompt so the orchestrator can parse the result +deterministically. ``` TURN1_OUTPUT_START @@ -756,29 +415,16 @@ 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 +That block is this subagent's entire final message — `status` selects the +branch, `testRunId` is the join key back to the CSV row / registry entry, and +the remaining fields paste straight into `flip()` or `recordTurn1()`. + +Agent-tool dispatches return immediately (fire-and-forget). Fire off every +representative's dispatch together, then **immediately proceed to Step 4's +evidence pre-fetch — do not wait for any of them.** + +As each subagent finishes, a task-notification carrying its `TURN1_OUTPUT` +block arrives. 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)` @@ -824,42 +470,20 @@ bookkeeping — no new tool calls needed for this part: 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. +5. **A subagent that never reports back fails open, not closed.** Step 5's + `readTurn1` returns nothing → Step 5 falls back to a fresh dispatch + (submit turn 1 from scratch, no `resume`/`turn1_result`). If the dead + subagent did reach `tfaRcaTurn`, that thread is orphaned — not a + correctness problem, just one wasted thread per failure. + +**Dispatch at most `concurrency` (from `config/rca.config.json`) 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). + +**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.** Pass `turn1PathFor(...)`'s path to Step 5 alongside `evidenceFilePath` — Step 5 must read it (`readTurn1(path, testRunId)`) before building each @@ -873,22 +497,12 @@ representative outcome for seeding siblings. **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. +issued.** **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 is NOT a "wait for Step 4b's subagents +to finish" gate — it only catches the case where Step 4b never happened at +all. **ORDER MATTERS: representative first, siblings only after it lands.** For each cluster, dispatch the representative, wait for its row to go terminal, then @@ -897,102 +511,44 @@ dispatch its siblings carrying `pre_seed` from 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. - -**"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. +Never hand-roll the seed: without this guard, siblings degenerate into full +independent investigations at representative-level cost. + +Drive the cluster work-list **`concurrency` at a time** — read `concurrency` from +`config/rca.config.json`, never hardcode a number: representatives deep, siblings +one-turn-confirm. Eagerly persist to the CSV/WAL (claim → heartbeat → flip) so the +run is resumable. Keep it a **rolling queue, not two rigid phases**: as each batch +returns, refill up to `concurrency` by mixing freed representatives' siblings (via +`siblingPreSeed`) with not-yet-dispatched representatives from other clusters — +never "all representatives, then all siblings," which idles a fast cluster's +siblings behind an unrelated slow representative. + +Dispatch path, in preference order: + +- **Direct Agent-tool dispatch** — **the default.** Dispatch + `tfa-rca:ai-tfa-coordinator` subagents in batches of `concurrency` (one message, up + to `concurrency` tool-use blocks), refilling per the rolling queue above. Honors the + JSON `concurrency` literally. Coordinator output flows back into the orchestrator's + context — kept affordable by the compact `RCA_OUTPUT` contract. A batch is a barrier + (the next batch waits for the slowest in the current one). + **This path has no code enforcing the Step 4b handoff — you are the enforcement.** + Before dispatching ANY representative, call `readTurn1(turn1PathFor(buildId, + stateDir), testRunId)` and fold the result into the prompt using this exact mapping + — 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, use the CSV row's result directly. A swapped field + is silently wrong, not rejected. +- **`workflows/rca-batch.mjs`** — **opt-in** (Claude Code, when the Workflow runtime is + available). Keeps coordinator output out of the orchestrator's context and gives + `resumeFromRunId` resumability + a progress UI. Runs fewer agents at once than direct + dispatch, so choose it when orchestrator context is the constraint (very large + builds) or you want the UI/resumability — not for throughput. +- **Sequential harness `lib/loop.mjs`** (`runRcaLoop`) — hosts without the Workflow + runtime and without Agent-tool fan-out, 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 @@ -1000,35 +556,24 @@ 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`. +reference-doc / lib path.** A coordinator is dispatched fresh with no guarantee +about its cwd. Every dispatch prompt must state `pluginRoot=<absolute path>` up +front and every reference-doc pointer must 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. +signatures for `lib/*.mjs` are documented at +`<pluginRoot>/skills/rca-build/references/api.md` — read it once if a signature is +needed; do not `grep`/`Read`/`cat` the `lib/` source to re-derive a signature +already documented there." **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. +Gate Part A — e.g. _"Use `<resolved-github-skill>` for every +product_code / deploy / ci ask. Use `<resolved-infra-skill>` for every infra +ask."_ Omitting a manifest-listed connector lets the coordinator infer repos +from workspace `git remote` or cwd, landing wrong PR attributions. **Coordinator prompts MUST also name the Step 4 evidence file.** Every dispatch prompt (representative and sibling alike) includes the absolute @@ -1045,7 +590,7 @@ 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`), +`contributeCodeEvidence`/`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 @@ -1062,36 +607,24 @@ 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. +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.** Include the plugin root in each +dispatch prompt so coordinators can invoke `bin/cached-exec.mjs` / +`bin/cached-mcp.mjs`, 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 +cache savings. **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. +`rca-evidence.<buildId>.contrib/<testRunId>.json`. `readEvidenceFile` folds +base + all shards into one view, applying shards in sorted order, with real +evidence taking precedence over a recorded `gap`. **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 @@ -1108,27 +641,16 @@ This plugin **never renders or writes a local RCA report, and never surfaces RCA detail in Claude.** The in-Claude output is a two-line completion notice plus the link — that is all. When every row is terminal: -1. Print the **completion summary** from the CSV (`lib/glimpse.mjs` → - `renderGlimpse`): `RCA analysis complete — build <id>` + a status count line - (`<N> tests · <R> resolved · <P> pending · <F> failed`). **Nothing per-test.** +1. Print a one-line **completion summary** by counting the CSV's terminal + states: `RCA analysis complete — build <id>` + `<N> tests · <R> resolved · + <P> pending · <F> failed`. **Nothing per-test.** 2. Call **`triggerRcaReport(buildUuid=<build id>, force=true)`** — **always pass `force=true`; never `force=false` in any case.** Forcing regenerates the release-readiness report from the RCAs completed so far, so the report is produced for this run's actual analysis even when only a subset of tests reached terminal RCA — instead of returning a stale/empty cached report or blocking on a bulk re-trigger of every test's RCA. -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: +3. Print the link line, verbatim shape: ``` Full report on the Test Observability UI: <viewReport> diff --git a/skills/rca-build/references/api.md b/skills/rca-build/references/api.md new file mode 100644 index 0000000..ce04219 --- /dev/null +++ b/skills/rca-build/references/api.md @@ -0,0 +1,81 @@ +# API reference — read this, don't grep the source + +The `lib/` and `bin/` surface the orchestrator and coordinator call. Load this +when you first need a signature (Step 2 onward) — not at gate time. Everything +here 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="") → <stateDir|tmpdir>/bstack-rca/rca-state.<buildId>.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/theme-clustering.mjs` + `lib/signature.mjs`** +``` +clustersFromThemes(rows, themesResult, testsByThemeId) → {rows, clusters}; server themes → clusters (empty themes → every test a singleton). Mutates cluster_id; caller persists via writeRows. +siblingPreSeed(csvPath, csvState, clusterId, repId) → {ok, pre_seed} | {ok:false, reason} +``` + +**Shared evidence — `lib/evidence-file.mjs`** +``` +evidencePathFor(buildId, stateDir="") initEvidenceFile(path, buildId, nowMs) +setCodeEvidence(path, repo, entry, nowMs) setLogsEvidence(path, workload, entry, nowMs) +setBaseline(path, baseline, suspectWindow, nowMs) setLocalRepos(path, localRepos, nowMs) +contributeCodeEvidence(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 (perms only, never deletes) +``` + +**Step 4b turn-1 pre-dispatch registry — `lib/turn1-registry.mjs`** +``` +turn1PathFor(buildId, stateDir="") → <stateDir|tmpdir>/bstack-rca/rca-turn1.<buildId>.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 +``` + +**Routing — `lib/routing.mjs`, `lib/evidence-cache.mjs`** +``` +loadConfig(configPath) buildManifest(config, discovered) routeAsks(asks, config, manifest) +resolveBaseline(lastGreenRef, fallbackRef) +``` + +**Commands — `bin/`** +``` +node bin/evidence-show.mjs <evidenceFile> [--summary | --prs | --repo <org/repo>] +node bin/repo-read.mjs <buildId> <writerId> <org/repo> <sha> <path> [--fetch] +node bin/cached-exec.mjs <buildId> <writerId> '<command>' (pipe OUTSIDE the wrapper) +node bin/cached-mcp.mjs <buildId> get|put <tool> '<argsJson>' +``` + +**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. diff --git a/skills/rca-build/references/clustering.md b/skills/rca-build/references/clustering.md index 9f3fe2c..2be2771 100644 --- a/skills/rca-build/references/clustering.md +++ b/skills/rca-build/references/clustering.md @@ -1,60 +1,50 @@ # 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* -instead of once per *test* turns the dominant cost from **O(tests) → O(distinct -causes)** — the only thing that makes "RCA for ALL failed tests, even thousands" -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*. - -## 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`. Cadence: one GET first, a single POST trigger only if no - themes exist yet (never re-fired), then GET every 3s up to a 90s wall-clock - ceiling — `ready: true` on `SUCCESS`, `ready: false` if the 90s is spent or - the status is `FAILED`/`ERROR`. 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. This is a **server-outage net, not the routine path - for un-computed builds.** The trigger endpoint is deployed, so - `getBuildFailureThemes` makes themes exist for a fresh build (one POST, same - call) and returns `ready: true` — a never-analyzed build no longer degrades - to signatures. `ready: false` now means the server genuinely couldn't produce - themes: still computing past the poll budget, a failure status, or - `status: "trigger-unavailable"` (the trigger call itself errored). Only then - does this fallback engage, keeping the run resilient when o11y is unavailable - rather than aborting or exploding to one coordinator per test. - -## The signature (fallback path only) - -Computed from the trimmed failure detail `listTestIds(includeFailureDetail=true)` -already returns on each row — **no extra probe turns**: - -``` -signature = normalize(failure_category) | normalize(error_summary) | normalize(file_path) -``` - -`normalize` folds the volatile tokens that make two instances of the *same* -failure look different: ISO timestamps, UUIDs, hex/memory addresses, `file:line:col`, -and bare numbers. So `timeout after 3000ms on node-7` and `timeout after 5000ms -on node-2` share a signature. - -A row with **no signal** (empty category, error, and path) is **not** merged into -a catch-all — it becomes its own singleton (`solo-<testRunId>`). Better an -un-clustered test than a wrong cluster. +Clustering runs the full collaborative loop once per *cause* instead of once per +*test* — **O(tests) → O(distinct causes)**. Every failed test still shows a +per-test RCA in the TRA dashboard; clustering collapses the *evidence hunt*, not +the *output*. + +## Source: the server's failure themes + +Clustering comes from one source — the server's failure themes — so the +`{ cluster_id, signature, members, representative, siblings }` shape is produced +the same way every run, and nothing downstream (the fan-out workflow, the +sequential harness) needs to branch on it. + +`lib/theme-clustering.mjs` → `clustersFromThemes(rows, themesResult, testsByThemeId)`, +fed from the `getBuildFailureThemes` / `listTestsInFailureTheme` MCP tools +(SKILL.md Step 3). `getBuildFailureThemes` makes themes exist, not just reads +them: if none have been computed it triggers computation (one POST, same call) +and polls `buildThemeWorkflow.status` — one GET first, a single POST trigger +only when no themes exist yet (never re-fired), then GET every 3s up to a 90s +ceiling. `ready: true` (SUCCESS) → real themes; `ready: false` (budget spent, +`FAILED`/`ERROR`, or `trigger-unavailable`) → the server couldn't group. The +grouping reflects the server's own root-cause analysis: two failures with an +identical error string but unrelated causes aren't conflated the way a text-only +guess would conflate them. + +**When the server returns no themes (`ready: false`)**, pass an empty +`buildThemes` to `clustersFromThemes` and every failed test falls through to its +own `solo-` cluster — i.e. **all tests become representatives**, each running a +full per-test loop. Correctness over the cost collapse: no local guessing. + +## Running it (Step 3) + +1. `getBuildFailureThemes(buildUuid=<build id>)` — triggers + polls in-call + (≤~90s, safe to await inline; cadence above). +2. **`ready: true`** → for each `buildThemes` entry, call + `listTestsInFailureTheme(buildUuid=<build id>, themeId=<buildFailureThemeId>)`, + following `nextCursor` to exhaustion for its member testRunIds, then + `clustersFromThemes(rows, themesResult, testsByThemeId)`. Any test the server + didn't assign still gets its own singleton. +3. **`ready: false`** → `clustersFromThemes(readRows(csvPath), { buildThemes: [] }, {})`. + +**Invariants.** `rows` MUST be `readRows(csvPath)` (the CSV Step 2 seeded), never a +`listTestIds` variable held over from earlier in the turn. `clustersFromThemes` +mutates `cluster_id` but does NOT persist — `writeRows(csvPath, rows)` before fan-out, +then verify: **if any row's `cluster_id` is empty, Step 3 did not take effect — do not +proceed.** ## Representative + siblings @@ -80,10 +70,6 @@ Distinct failures can share an error string. A sibling's pre-seed turn is a test's logs) → the sibling **falls back to its own full loop**. The representative's cause is never stamped onto a sibling without log confirmation. -This keeps correctness independent of the cost optimization: worst case, every -sibling runs its own full loop (same as no clustering); best case, one deep run -covers the whole cluster. - ## Singletons A cluster of one is just a plain per-test loop — no pre-seed, no confirm step. diff --git a/skills/rca-build/references/evidence-routing.md b/skills/rca-build/references/evidence-routing.md index 2c3ba5b..c84c766 100644 --- a/skills/rca-build/references/evidence-routing.md +++ b/skills/rca-build/references/evidence-routing.md @@ -9,9 +9,7 @@ The core contract: **TFA owns logs; the client agent owns everything else.** The coordinator never seeds logs and never fulfills a `test_logs` ask. Every other `evidenceType` routes to a capability that is gathered via **whatever skill/tool the client actually has** for it (discovered **and validated** once into the -capability manifest — see `SKILL.md` § Gate Part A). There are **no `kubectl` / -`chitragupta` / -`bifrost` literals here** — that is the whole point of going generic. +capability manifest — see `SKILL.md` § Gate Part A). **Contents:** [How asks are processed](#how-a-turns-asks-are-processed) · [Routing table](#routing-table-capability-not-tool) · @@ -76,10 +74,8 @@ suspect that could not have caused the failure. (Full protocol: U9 / ## Digest format -The single most important discipline: **digested input, not raw dumps.** Every -turn's `message` loads into the agent's context *and* is sent to TFA; a raw log -tail or full PR diff blows both budgets and degrades TFA's reasoning. Supply the -*findings*, not the *haystack*. +**Digested input, not raw dumps.** Every turn's `message` loads into the agent's +context *and* is sent to TFA. Supply the *findings*, not the *haystack*. ### Per-ask block shape — `ask → found → snippet/link` @@ -96,21 +92,16 @@ and unfulfillable variants) — copy it, don't retype it. Shape: | Field / scope | Soft target | Hard ceiling | On exceed | |---|---|---|---| -| `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 | +| `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 | | 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 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. +whole-message ceiling also honors `turnMessageMaxChars` from +`config/rca.config.json` (the tool caps `message` at 5000 chars). ### What never goes in a digest @@ -143,11 +134,10 @@ does not pre-empt that decision. ## Capability manifest (built once, at the gate) -Rather than re-discover "is there a kibana skill?" on every ask across every -test, Gate Part A enumerates **and probe-validates** the client's connectors -**once** up front into a manifest (`lib/routing.mjs` → `buildManifest`). -`valid` maps to `available: true`; `invalid`/`absent` map to `available: false` -(a recorded gap): +Gate Part A enumerates **and probe-validates** the client's connectors **once** +up front into a manifest (`lib/routing.mjs` → `buildManifest`). `valid` maps to +`available: true`; `invalid`/`absent` map to `available: false` (a recorded +gap): ``` { github: {available: true, via: "gh"}, infra: {available: true, via: "kubectl"}, logs: {available: false}, ... } diff --git a/skills/rca-build/references/github-evidence.md b/skills/rca-build/references/github-evidence.md index 2960a9c..b07e9ce 100644 --- a/skills/rca-build/references/github-evidence.md +++ b/skills/rca-build/references/github-evidence.md @@ -1,12 +1,10 @@ # GitHub evidence — what to gather, and how to rule a suspect OUT -The worst automated-RCA outcome is **confidently blaming an innocent PR**. This -file is the contract for `product_code` / `deploy` / `ci` asks (the `github` -capability): the **exact** evidence to gather, and a **falsification protocol** -that tries to *disprove* each suspect before it enters `related_prs`. +This file is the contract for `product_code` / `deploy` / `ci` asks (the +`github` capability): the **exact** evidence to gather, and a **falsification +protocol** that tries to *disprove* each suspect before it enters `related_prs`. -> We do **not** ship a GitHub forensics harness or MCP tool. We specify what's -> needed and use whatever the client already has — **GitHub MCP if available, +> Uses whatever the client already has — **GitHub MCP if available, > else `gh`, else degrade** to an `unavailable` block. **Contents:** [Capability discovery](#capability-discovery-in-order) · @@ -55,24 +53,15 @@ TFA (a gate-recorded gap). 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). +below, and each candidate PR's falsification check. 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. +message**. 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) @@ -90,13 +79,10 @@ 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. +Every gather call should already be filtered to the field(s) the ask needs, +not filtered after the fact by reading past the noise. This applies to +whichever connector resolved for `github` and equally to `infra` gather +calls (`kubectl` or whatever the manifest resolved to). | Need | Don't — pulls the whole object | Do — projects to the field(s) the ask needs | |---|---|---| @@ -108,14 +94,10 @@ manifest resolved to), since the failure mode is identical. | 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. +**Never run the unfiltered form "to see the shape first."** 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 diff --git a/skills/rca-build/templates/evidence-block.md b/skills/rca-build/templates/evidence-block.md index 97b41f7..fa0bf49 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, ≤ 80 chars> +ASK: <verbatim `what` from the TfaAsk, ≤ 120 chars> TYPE: <evidenceType> FOUND: <yes | no | partial> -SUMMARY: <1 sentence — the finding, in the agent's words. ≤ 80 chars> +SUMMARY: <1–3 sentences — the finding, in the agent's words. ≤ 400 chars> SNIPPET: - <the load-bearing excerpt only, ≤ 4 lines — see size caps. Omit if a LINK fully carries it.> + <the load-bearing excerpt only — 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 deleted file mode 100644 index e903052..0000000 --- a/tests/build-cleanup.test.mjs +++ /dev/null @@ -1,88 +0,0 @@ -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/coverage-glimpse.test.mjs b/tests/coverage-glimpse.test.mjs deleted file mode 100644 index 3bc0e48..0000000 --- a/tests/coverage-glimpse.test.mjs +++ /dev/null @@ -1,93 +0,0 @@ -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { coverageStamp, classifyCoverage } from "../lib/coverage.mjs"; -import { renderGlimpse } from "../lib/glimpse.mjs"; - -// ---- coverage stamp -------------------------------------------------------- - -test("full coverage keeps TFA confidence", () => { - const s = coverageStamp({ - asksFulfilled: ["product_code"], - asksUnavailable: [], - tfaConfidence: "high", - }); - assert.equal(s.coverage, "full"); - assert.equal(s.band, "high"); -}); - -test("partial coverage caps a high TFA confidence at medium", () => { - const s = coverageStamp({ - asksFulfilled: ["product_code"], - asksUnavailable: ["kibana"], - tfaConfidence: "high", - }); - assert.equal(s.coverage, "partial"); - assert.equal(s.band, "medium"); - assert.deepEqual(s.unavailable, ["kibana"]); -}); - -test("thin coverage (nothing fulfilled, gaps) caps at low", () => { - const s = coverageStamp({ - asksFulfilled: [], - asksUnavailable: ["infra", "metrics"], - tfaConfidence: "high", - }); - assert.equal(s.coverage, "thin"); - assert.equal(s.band, "low"); -}); - -test("unknown TFA confidence floors to low even at full coverage", () => { - const s = coverageStamp({ asksFulfilled: [], asksUnavailable: [], tfaConfidence: "unknown" }); - assert.equal(s.coverage, "full"); - assert.equal(s.band, "low"); -}); - -test("classifyCoverage dedupes and handles empties", () => { - assert.equal(classifyCoverage(["a", "a"], []), "full"); - assert.equal(classifyCoverage([], ["x"]), "thin"); -}); - -// ---- glimpse (the ONLY in-client output — no local report) ------------------ - -test("empty batch renders a valid glimpse, no crash", () => { - const txt = renderGlimpse([], { buildId: "b1" }); - assert.match(txt, /No failed tests analyzed/); -}); - -test("glimpse is a completion notice with counts — NO per-test detail", () => { - const rows = [ - { - testRunId: "101", - cluster_id: "c1", - rca_done: "resolved", - confidence: "high", - root_cause: "PR #7421 tightened validator", - related_prs: "#7421", - }, - { testRunId: "102", cluster_id: "c1", rca_done: "pending-resume" }, - { testRunId: "103", cluster_id: "c2", rca_done: "failed" }, - ]; - const txt = renderGlimpse(rows, { buildId: "b1" }); - assert.match(txt, /RCA analysis complete — build b1/); - assert.match(txt, /3 test\(s\)/); - assert.match(txt, /1 resolved/); - assert.match(txt, /1 pending/); // pending-resume buckets to "pending" - assert.match(txt, /1 failed/); - // The trim: no per-test lines, no root cause, no PRs, no testRunIds leak. - assert.doesNotMatch(txt, /7421|PR #|→|101|102|103|c1|c2/); -}); - -test("glimpse never leaks a verbose root_cause, however long", () => { - const rows = [ - { - testRunId: "1", - cluster_id: "solo-1", - rca_done: "resolved", - confidence: "medium", - root_cause: `line one\nline two ${"x".repeat(200)}`, - }, - ]; - const txt = renderGlimpse(rows); - assert.doesNotMatch(txt, /line one|line two|xxxx/); // cause stays out of Claude - assert.match(txt, /1 test\(s\) · 1 resolved/); -}); diff --git a/tests/csv-state.test.mjs b/tests/csv-state.test.mjs index 6070ef4..fe9e8aa 100644 --- a/tests/csv-state.test.mjs +++ b/tests/csv-state.test.mjs @@ -288,28 +288,3 @@ test("flipping to pending-resume without a turnId warns loudly", () => { // "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 index b9a84d8..60a25b2 100644 --- a/tests/evidence-file.test.mjs +++ b/tests/evidence-file.test.mjs @@ -10,15 +10,16 @@ import { readEvidenceFile, writeEvidenceFile, setBaseline, - setGithubEvidence, + setCodeEvidence, setLogsEvidence, - contributeGithubEvidence, + contributeCodeEvidence, contributeLogsEvidence, contribDirFor, contribPathFor, readBaseFile, hasTrustworthyPrList, recomputeCoverage, + assertGithubEntry, } from "../lib/evidence-file.mjs"; let dir; @@ -30,6 +31,39 @@ beforeEach(() => { }); afterEach(() => rmSync(dir, { recursive: true, force: true })); +// --- assertGithubEntry: the write-boundary guard that would have caught the +// real prod bug (a hand-rolled {deployState, prCount5d, topPRs} blob that +// readers ignore because they only read prsInWindow). --- + +test("assertGithubEntry: rejects the exact prod mis-shape (topPRs/prCount5d)", () => { + assert.throws( + () => assertGithubEntry({ deployState: { sha: "x" }, prCount5d: 6, topPRs: [] }, "repo-A"), + /unknown key\(s\) \[prCount5d, topPRs\]/, + ); +}); + +test("assertGithubEntry: accepts the canonical shape", () => { + assert.doesNotThrow(() => + assertGithubEntry({ deployState: { sha: "x" }, prsInWindow: [{ pr: 1, files: ["a.js"] }], prsSearched: true, gap: null }, "repo-A"), + ); + assert.doesNotThrow(() => assertGithubEntry({ gap: "unreachable" }, "repo-A")); + assert.doesNotThrow(() => assertGithubEntry({ deployState: null }, "repo-A")); +}); + +test("assertGithubEntry: rejects non-object and non-array prsInWindow", () => { + assert.throws(() => assertGithubEntry(null, "r"), /must be an object/); + assert.throws(() => assertGithubEntry([], "r"), /must be an object/); + assert.throws(() => assertGithubEntry({ prsInWindow: "nope" }, "r"), /prsInWindow must be an array/); +}); + +test("setCodeEvidence: propagates the guard — a mis-shaped entry throws, no dead file shipped", () => { + initEvidenceFile(file, "b1", 1000); + assert.throws( + () => setCodeEvidence(file, "repo-A", { deployState: { sha: "x" }, topPRs: [{ number: 1 }] }, 2000), + /unknown key\(s\) \[topPRs\]/, + ); +}); + 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"))); @@ -64,39 +98,39 @@ test("initEvidenceFile creates the file with the given buildId", () => { 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); + setCodeEvidence(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); +test("setCodeEvidence and setLogsEvidence coexist without clobbering each other", () => { + setCodeEvidence(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); +test("setCodeEvidence for a second repo does not disturb the first", () => { + setCodeEvidence(file, "org/a", { gap: null, deployState: { block: "a" } }, 1000); + setCodeEvidence(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); +test("setCodeEvidence twice for the SAME repo overwrites only that repo", () => { + setCodeEvidence(file, "org/a", { gap: null, deployState: { block: "old" } }, 1000); + setCodeEvidence(file, "org/b", { gap: null, deployState: { block: "b" } }, 1000); + setCodeEvidence(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); + setCodeEvidence(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 }); @@ -105,7 +139,7 @@ test("setBaseline records baseline and suspectWindow without touching github/log }); test("recomputeCoverage: a covered repo/workload has no gap; a missing one is gapped", () => { - setGithubEvidence(file, "org/a", { gap: null, deployState: { block: "a" } }, 1000); + setCodeEvidence(file, "org/a", { gap: null, deployState: { block: "a" } }, 1000); setLogsEvidence(file, "w1", { gap: null, kubectlSweep: { block: "w1" } }, 1000); const coverage = recomputeCoverage( file, @@ -119,14 +153,14 @@ test("recomputeCoverage: a covered repo/workload has no gap; a missing one is ga }); 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); + setCodeEvidence(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); + setCodeEvidence(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"]); @@ -134,14 +168,14 @@ test("recomputeCoverage persists onto the file (readable afterwards)", () => { 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); + setCodeEvidence(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", { + setCodeEvidence(file, "org/a", { gap: null, deployState: { block: "base" } }, 1000); + contributeCodeEvidence(file, "3895581484", "org/a", { deployState: { block: "coordinator's full diff" }, }, 2000); // base is untouched... @@ -162,14 +196,14 @@ test("contribPathFor sanitizes a hostile writerId", () => { }); test("CONCURRENCY: two writers on the same repo both survive (no lost update)", () => { - setGithubEvidence(file, "org/a", { + setCodeEvidence(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); + contributeCodeEvidence(file, "writerA", "org/a", { prsInWindow: [{ pr: "#2", by: "A" }] }, 2000); + contributeCodeEvidence(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 }); @@ -183,8 +217,8 @@ test("CONCURRENCY: two writers on the same workload both survive", () => { }); test("fold: real contributed evidence beats a base-recorded gap", () => { - setGithubEvidence(file, "org/a", { gap: "gh auth failed" }, 1000); - contributeGithubEvidence(file, "w1", "org/a", { + setCodeEvidence(file, "org/a", { gap: "gh auth failed" }, 1000); + contributeCodeEvidence(file, "w1", "org/a", { gap: null, deployState: { block: "reachable after all" }, }, 2000); const entry = readEvidenceFile(file).github["org/a"]; @@ -193,16 +227,16 @@ test("fold: real contributed evidence beats a base-recorded gap", () => { }); 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); + setCodeEvidence(file, "org/a", { gap: null, deployState: { block: "real base evidence" } }, 1000); + contributeCodeEvidence(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", { + setCodeEvidence(file, "org/a", { gap: null, prsInWindow: [{ pr: "#9011", verdict: "unassessed", files: null }], }, 1000); - contributeGithubEvidence(file, "w1", "org/a", { + contributeCodeEvidence(file, "w1", "org/a", { prsInWindow: [{ pr: "#9011", verdict: "supported", files: ["Foo.java"] }], }, 2000); const prs = readEvidenceFile(file).github["org/a"].prsInWindow; @@ -211,7 +245,7 @@ test("fold: same PR number contributed later wins (deeper finding replaces place }); test("fold: contributing a repo the pre-fetch never named", () => { - contributeGithubEvidence(file, "w1", "org/brand-new", { + contributeCodeEvidence(file, "w1", "org/brand-new", { prsInWindow: [{ pr: "#8912", verdict: "supported" }], }, 1000); assert.equal(readEvidenceFile(file).github["org/brand-new"].prsInWindow[0].pr, "#8912"); @@ -225,18 +259,18 @@ test("fold: clusterIds union across base and multiple shards", () => { }); 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); + setCodeEvidence(file, "org/a", { gap: null, deployState: { block: "base" } }, 1000); + contributeCodeEvidence(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); + setCodeEvidence(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); + contributeCodeEvidence(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, []); @@ -247,8 +281,8 @@ test("recomputeCoverage counts a coordinator-filled gap as covered", () => { // 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); + setCodeEvidence(file, "org/never-searched", { gap: null, deployState: { block: "d" }, prsInWindow: [] }, 1000); + setCodeEvidence(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". @@ -257,9 +291,9 @@ test("empty prsInWindow is NOT coverage unless the search is recorded", () => { }); 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); + setCodeEvidence(file, "org/a", { gap: null, prsInWindow: [] }, 1000); + setCodeEvidence(file, "org/b", { gap: null, prsInWindow: [], prsSearched: true }, 1000); + setCodeEvidence(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); @@ -267,26 +301,26 @@ test("hasTrustworthyPrList distinguishes searched-empty from never-populated", ( }); test("contributing a PR list records that the search actually ran", () => { - contributeGithubEvidence(file, "w1", "org/a", { prsInWindow: [] }, 1000); + contributeCodeEvidence(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); + setCodeEvidence(file, "org/a", { gap: null, prsInWindow: [{ pr: "#1" }], prsSearched: true }, 1000); + contributeCodeEvidence(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); + setCodeEvidence(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); + setCodeEvidence(file, "org/a", { gap: null, deployState: { block: "private PR detail" } }, 1000); + contributeCodeEvidence(file, "w1", "org/a", { prsInWindow: [{ pr: "#1" }] }, 2000); assert.equal(statSync(file).mode & 0o777, 0o600); assert.equal(statSync(contribPathFor(file, "w1")).mode & 0o777, 0o600); }); @@ -342,16 +376,16 @@ test("stalenessOf refuses to call a future timestamp fresh", async () => { // 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 } = + const { evidencePathFor, initEvidenceFile, setCodeEvidence, 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", { + setCodeEvidence(p, "org/explicit", { deployState: { sha: "abc1234", summary: "" } }, 2); + setCodeEvidence(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); + setCodeEvidence(p, "org/none", { deployState: { summary: "no sha here" } }, 4); const { pins, source } = deployShas(p); assert.equal(pins["org/explicit"], "abc1234"); @@ -373,12 +407,12 @@ test("deployShas prefers the explicit field and falls back to the summary", asyn // 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 } = + const { evidencePathFor, initEvidenceFile, contributeCodeEvidence, readEvidenceFile } = await import("../lib/evidence-file.mjs"); const p = evidencePathFor("b-prkey", dir); initEvidenceFile(p, "b-prkey", 1); - contributeGithubEvidence(p, "w1", "org/r", { + contributeCodeEvidence(p, "w1", "org/r", { prsSearched: true, prsInWindow: [{ title: "first" }, { title: "second" }, { title: "third" }], }, 2); @@ -391,13 +425,13 @@ test("numberless PRs do not collapse into one another", async () => { test("numbered PRs still merge across writers, string or numeric", async () => { const dir = mkdtempSync(join(tmpdir(), "rca-prnum-")); - const { evidencePathFor, initEvidenceFile, contributeGithubEvidence, readEvidenceFile } = + const { evidencePathFor, initEvidenceFile, contributeCodeEvidence, 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); + contributeCodeEvidence(p, "w1", "org/r", { prsInWindow: [{ pr: "#10", title: "a" }] }, 2); + contributeCodeEvidence(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"); @@ -411,12 +445,12 @@ test("numbered PRs still merge across writers, string or numeric", async () => { // 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 } = + const { evidencePathFor, initEvidenceFile, setCodeEvidence, 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); + setCodeEvidence(p, "org/r", { deployState: { sha: "abc1234" } }, 2); const raw = readFileSync(p, "utf8"); const head = raw.slice(0, 400); @@ -437,11 +471,11 @@ test("a raw read of the base file announces that it is partial", async () => { 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 { evidencePathFor, initEvidenceFile, setCodeEvidence } = 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); + for (let i = 0; i < 3; i++) setCodeEvidence(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"); diff --git a/tests/prefetch-prs.test.mjs b/tests/prefetch-prs.test.mjs new file mode 100644 index 0000000..624cc13 --- /dev/null +++ b/tests/prefetch-prs.test.mjs @@ -0,0 +1,30 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { normalizePrs } from "../bin/prefetch-prs.mjs"; + +// normalizePrs maps `gh pr list --json …,files` output to the canonical +// prsInWindow rows — the shape readers actually consume. The prod bug was a +// hand-rolled `topPRs` that dropped `files`; this keeps `files` first-class. + +test("normalizePrs: keeps pr number, metadata, and flattens files to paths", () => { + const raw = [ + { number: 7867, title: "TRAP-4119", mergedAt: "2026-08-20T12:26:06Z", url: "u1", + files: [{ path: "a/b.js" }, { path: "c.js" }] }, + ]; + assert.deepEqual(normalizePrs(raw), [ + { pr: 7867, title: "TRAP-4119", mergedAt: "2026-08-20T12:26:06Z", url: "u1", files: ["a/b.js", "c.js"] }, + ]); +}); + +test("normalizePrs: tolerates string-file arrays and missing fields", () => { + const raw = [{ number: 1, files: ["x.ts"] }, { number: 2 }]; + const out = normalizePrs(raw); + assert.deepEqual(out[0].files, ["x.ts"]); + assert.deepEqual(out[1].files, []); + assert.equal(out[1].pr, 2); +}); + +test("normalizePrs: non-array input yields empty list", () => { + assert.deepEqual(normalizePrs(null), []); + assert.deepEqual(normalizePrs(undefined), []); +}); diff --git a/tests/signature.test.mjs b/tests/signature.test.mjs index 02dbfd0..a52605e 100644 --- a/tests/signature.test.mjs +++ b/tests/signature.test.mjs @@ -3,12 +3,7 @@ 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, - selectRepresentative, - clusterRows, -} from "../lib/signature.mjs"; +import { selectRepresentative, siblingPreSeed } from "../lib/signature.mjs"; function row(id, extra = {}) { return { @@ -21,50 +16,6 @@ function row(id, extra = {}) { }; } -test("normalize folds timestamps, uuids, hex, line:col, and numbers", () => { - assert.equal(normalize("Error at line :42:7"), "error at line :<line>"); - assert.equal(normalize("got 500 at 0xAF3"), "got <n> at <hex>"); - assert.equal( - normalize("failed 2026-06-23T10:00:00Z"), - "failed <ts>", - ); -}); - -test("identical category+error+path → same cluster", () => { - const { clusters } = clusterRows([row(1), row(2)]); - assert.equal(clusters.length, 1); - assert.equal(clusters[0].members.length, 2); -}); - -test("numbers in the error are folded so siblings still cluster", () => { - const a = row(1, { error_summary: "timeout after 3000ms on node-7" }); - const b = row(2, { error_summary: "timeout after 5000ms on node-2" }); - assert.equal(computeSignature(a), computeSignature(b)); - const { clusters } = clusterRows([a, b]); - assert.equal(clusters.length, 1); -}); - -test("distinct failures → distinct clusters", () => { - const a = row(1, { error_summary: "null pointer in Foo" }); - const b = row(2, { error_summary: "connection refused" }); - const { clusters } = clusterRows([a, b]); - assert.equal(clusters.length, 2); -}); - -test("rows with no signal become their own singletons (no catch-all merge)", () => { - const a = { testRunId: "1", failure_category: "", error_summary: "", file_path: "" }; - const b = { testRunId: "2", failure_category: "", error_summary: "", file_path: "" }; - const { clusters } = clusterRows([a, b]); - assert.equal(clusters.length, 2); - assert.ok(clusters.every((c) => c.cluster_id.startsWith("solo-"))); -}); - -test("singleton cluster has a representative and no siblings", () => { - const { clusters } = clusterRows([row(1)]); - assert.equal(clusters[0].siblings.length, 0); - assert.equal(clusters[0].representative.testRunId, "1"); -}); - test("representative is deterministic: non-flaky, then smallest testRunId", () => { const members = [ row(5, { is_flaky: "true" }), @@ -74,46 +25,9 @@ test("representative is deterministic: non-flaky, then smallest testRunId", () = assert.equal(selectRepresentative(members).testRunId, "7"); }); -test("clusterRows stamps cluster_id onto every row", () => { - const rows = [row(1), row(2, { error_summary: "different" })]; - clusterRows(rows); - 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" } }, diff --git a/tests/state-dir.test.mjs b/tests/state-dir.test.mjs index 1613785..e15b0c0 100644 --- a/tests/state-dir.test.mjs +++ b/tests/state-dir.test.mjs @@ -1,9 +1,9 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { mkdtempSync, rmSync, mkdirSync, writeFileSync, chmodSync, statSync, existsSync, utimesSync } from "node:fs"; +import { mkdtempSync, rmSync, mkdirSync, writeFileSync, chmodSync, statSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { hardenStateDir, pruneStateDir } from "../lib/state-dir.mjs"; +import { hardenStateDir } from "../lib/state-dir.mjs"; const mode = (p) => statSync(p).mode & 0o777; @@ -50,53 +50,3 @@ test("hardenStateDir is idempotent and safe on a missing dir", () => { 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/tool-cache.test.mjs b/tests/tool-cache.test.mjs index 56e22da..2e6cd9c 100644 --- a/tests/tool-cache.test.mjs +++ b/tests/tool-cache.test.mjs @@ -5,7 +5,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { toolCacheDirFor, cacheKey, mcpCacheKey, cacheGet, cachePut, cacheStats, - isCacheable, isCacheableMcp, isRunnable, redact, tokenize, splitPipeline, + isCacheable, isCacheableMcp, isImmutableRead, isRunStableRead, redact, banner, } from "../lib/tool-cache.mjs"; let dir; @@ -59,11 +59,6 @@ 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) + '"}'; @@ -80,29 +75,6 @@ test("redact still catches a bare Bearer token and a key=value secret", () => { 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); @@ -118,6 +90,8 @@ test("cacheStats counts entries", () => { assert.equal(s.bytes, 8); }); +// ---- isCacheable: mutation denylist ---------------------------------------- + test("isCacheable rejects mutating shell commands", () => { assert.equal(isCacheable("gh api repos/a"), true); assert.equal(isCacheable("kubectl get pods"), true); @@ -129,96 +103,86 @@ test("isCacheable rejects mutating shell commands", () => { 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); -}); +// ---- isImmutableRead: the new cacheability predicate ----------------------- -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("isImmutableRead: gh api with /git/ path is cacheable", () => { + assert.equal(isImmutableRead("gh api repos/o/r/git/blobs/abc123"), true); + assert.equal(isImmutableRead("gh api repos/o/r/git/trees/main"), true); + assert.equal(isImmutableRead("gh api repos/o/r/git/commits/abc"), 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("isImmutableRead: gh api with ?ref=<40-hex-sha> is cacheable", () => { + const sha = "a".repeat(40); + assert.equal(isImmutableRead(`gh api repos/o/r/contents/f?ref=${sha}`), true); + assert.equal(isImmutableRead(`gh api 'repos/o/r/contents/f?ref=${sha}&other=1'`), true); }); -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("isImmutableRead: unpinned gh api is NOT cacheable", () => { + assert.equal(isImmutableRead("gh api repos/o/r/pulls/123"), false); + assert.equal(isImmutableRead("gh api repos/o/r/contents/f"), false); + assert.equal(isImmutableRead("gh api repos/o/r/contents/f?ref=main"), false); }); -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("isImmutableRead: git show/cat-file/ls-tree/log with sha is cacheable", () => { + const sha = "b".repeat(40); + assert.equal(isImmutableRead(`git show ${sha}:path/to/file`), true); + assert.equal(isImmutableRead(`git cat-file -p ${sha}`), true); + assert.equal(isImmutableRead(`git ls-tree ${sha}`), true); + assert.equal(isImmutableRead(`git log ${sha} --oneline`), true); }); -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("isImmutableRead: git commands without sha are NOT cacheable", () => { + assert.equal(isImmutableRead("git show HEAD:path/to/file"), false); + assert.equal(isImmutableRead("git log main --oneline"), false); + assert.equal(isImmutableRead("git diff"), false); + assert.equal(isImmutableRead("git status"), false); + assert.equal(isImmutableRead("git branch"), 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"]); +test("isImmutableRead: kubectl and curl are never cacheable (live state)", () => { + assert.equal(isImmutableRead("kubectl get pods -n regression"), false); + assert.equal(isImmutableRead("curl https://example.com"), false); }); -// 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"); +// ---- isRunStableRead: repo reads that don't change within one build RCA ----- - 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"); +test("isRunStableRead: gh pr view/diff/list by number are cacheable", () => { + assert.equal(isRunStableRead("gh pr view 53786 --repo browserstack/frontend --json files,title"), true); + assert.equal(isRunStableRead("gh pr diff 53786 --repo browserstack/frontend"), true); + assert.equal(isRunStableRead("gh pr list -R browserstack/frontend"), true); +}); + +test("isRunStableRead: gh search and gh api repo reads are cacheable", () => { + assert.equal(isRunStableRead('gh search code "env.js" --repo browserstack/frontend'), true); + assert.equal(isRunStableRead("gh api repos/o/r/contents/apps/o11y/index.html"), true); + assert.equal(isRunStableRead("gh api repos/o/r/pulls/123"), true); +}); - 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("isRunStableRead: gh api writes are NOT run-stable", () => { + assert.equal(isRunStableRead("gh api -X POST repos/o/r/pulls"), false); + assert.equal(isRunStableRead("gh api --method PATCH repos/o/r/pulls/1"), false); }); -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("isRunStableRead: read-only git (no sha) is cacheable", () => { + assert.equal(isRunStableRead("git show HEAD:path/to/file"), true); + assert.equal(isRunStableRead("git log main --oneline"), true); + assert.equal(isRunStableRead("git diff main...HEAD"), true); }); -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("isRunStableRead: live state and mutations are NOT run-stable", () => { + assert.equal(isRunStableRead("kubectl get pods -n regression"), false); + assert.equal(isRunStableRead("kubectl logs pod-x"), false); + assert.equal(isRunStableRead("curl https://example.com"), false); + assert.equal(isRunStableRead("gh pr create --title x"), false); }); -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("isImmutableRead: gh pr list/view are NOT cacheable (mutable state)", () => { + assert.equal(isImmutableRead("gh pr list -R o/r"), false); + assert.equal(isImmutableRead("gh pr view 123"), false); }); +// ---- MCP ------------------------------------------------------------------- + test("MCP: stateful tools are never cacheable", () => { assert.equal(isCacheableMcp("mcp__grafana__query_loki_logs"), true); assert.equal(isCacheableMcp("mcp__browserstack__listTestIds"), true); @@ -248,12 +212,12 @@ test("an MCP result round-trips through the shared store", () => { assert.equal(cacheGet(dir, k).stdout, "0 rows, clean"); }); +// ---- Permissions ----------------------------------------------------------- + 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); }); @@ -270,3 +234,10 @@ test("CONCURRENCY: same key written twice stays readable and consistent", () => cachePut(dir, k, { command: "gh api repos/a", writerId: "w2", stdout: "same-bytes" }, 2000); assert.equal(cacheGet(dir, k).stdout, "same-bytes"); }); + +// ---- banner ---------------------------------------------------------------- + +test("banner is exported and callable", () => { + // Just verify it doesn't throw when called without a logPath + assert.doesNotThrow(() => banner("[test]", "")); +}); diff --git a/tests/turn1-registry.test.mjs b/tests/turn1-registry.test.mjs index ab053f9..504c274 100644 --- a/tests/turn1-registry.test.mjs +++ b/tests/turn1-registry.test.mjs @@ -9,7 +9,6 @@ import { recordTurn1, readTurn1, readAllTurn1, - deleteTurn1Registry, } from "../lib/turn1-registry.mjs"; const mode = (p) => statSync(p).mode & 0o777; @@ -115,14 +114,3 @@ test("the registry file and its directory are owner-only (0600 / 0700)", () => { 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 index 4138bf6..2969b24 100644 --- a/tests/wiring.test.mjs +++ b/tests/wiring.test.mjs @@ -79,22 +79,27 @@ test("gate-critical lib exports are actually invoked outside tests", () => { // 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"); + // The API surface lives in references/api.md (loaded on-demand at Step 2+); + // SKILL.md only points at it. Scan both so the drift guard still fires. + const skill = + readFileSync(join(ROOT, "skills/rca-build/SKILL.md"), "utf8") + + "\n" + + readFileSync(join(ROOT, "skills/rca-build/references/api.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", + "hasTrustworthyPrList", "stalenessOf", "makeEvidenceCache", "assertGithubEntry", + "replaySubmit", "replayRead", "selectRepresentative", "localCloneFor", "hasCommit", "ensureCommit", "classifyCoverage", "coverageStamp", "orderAsks", "routeAsk", - "unavailableCapabilities", "renderGlimpse", "toolCacheDirFor", "cacheKey", - "isCacheable", "splitPipeline", + "unavailableCapabilities", "toolCacheDirFor", "cacheKey", + "isCacheable", // 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", + "isImmutableRead", "isRunStableRead", "isCacheableMcp", "redact", "cacheGet", + "cachePut", "cacheStats", "mcpCacheKey", "banner", ]); const undocumented = []; diff --git a/workflows/rca-batch.mjs b/workflows/rca-batch.mjs index 9d04a92..2330f52 100644 --- a/workflows/rca-batch.mjs +++ b/workflows/rca-batch.mjs @@ -77,7 +77,7 @@ const shared = [ `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.`, + `The file is read-write: after any live gather that fills a gap or goes deeper than what was there, write it back via contributeCodeEvidence/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.`, @@ -174,11 +174,9 @@ 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 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. +// representative is still looping. Parallelism on this path is capped by the +// Workflow runtime itself (a machine-dependent limit), not by config.concurrency +// — the runtime queues anything beyond its own cap regardless of the JSON value. const results = await pipeline( clusters, (cluster) =>