Feat/generic rca agent plugin v3 - #2
Open
Dave3130 wants to merge 51 commits into
Open
Conversation
Strips product-specific artifacts that leaked into the generic plugin from the nl2steps latency-instrumentation runs: the hardcoded "at least 5 kubectl calls" busywork floor and the hardcoded VictoriaLogs Grafana datasource UID in the shared coordinator prompt, plus de-hardcoded remaining nl2steps/o11y/tcm example references in SKILL.md and ai-tfa-coordinator.md down to neutral placeholders. Restores concurrency to 20 (the profiling-backed fleet ceiling) instead of the untested 50. Also gitignores .DS_Store/*.code-workspace to keep local editor artifacts out of the repo. The generic "mandatory connector sweep" mechanism itself (a connector skill may declare a compulsory check, routed by capability) is kept — only the instrumentation-specific numbers/UID were testing artifacts.
Removes the "proactive, never ask-gated" mandatory connector sweep mechanism (Operating Principle 0 + loop step 0.5 in ai-tfa-coordinator.md, the shared-prompt reminder in rca-batch.mjs, and the mandatory_checks field in the RCA_OUTPUT schema/contract and SKILL.md's closing bullet). Forcing evidence gathering into turn 1 regardless of whether TFA actually asked for it bloats every coordinator's first message and burns tokens for evidence that may not be needed. Evidence gathering is now purely reactive to TFA's own NEEDS_INFO asks, routed by capability as before.
… all coordinators Each dispatched ai-tfa-coordinator previously re-ran its own turn-1 PR-window search and kubectl/VictoriaLogs sweep independently, even though that evidence is a property of the build, not of any one test. Add lib/evidence-file.mjs (a build-scoped JSON artifact, keyed by repo and workload, mirroring csv-state.mjs's path convention) so the orchestrator gathers this once and every representative/sibling reads it before falling back to a live call. Validated against a real build: two re-run tests landed the same root causes (including the same culprit PR) at 69-70% fewer tokens and 90-94% fewer tool calls than the original per-coordinator sweep. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Self-imposed plugin budget, well under the tfaRcaTurn tool's actual 5000-char hard cap. Retunes the per-block digest caps (SUMMARY, SNIPPET) proportionally so real turn-1 messages fit the new budget instead of relying on ad-hoc truncation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d file The pre-fetch file was read-only from a coordinator's side — when a live gather filled a gap or went deeper than the pre-fetch had (a full diff instead of a summary, a PR the pre-fetch never named), that extra work died with the coordinator instead of benefiting its own siblings or other clusters sharing the same repo/workload. Add mergeGithubEvidence/mergeLogsEvidence (read-modify-write, dedupe PRs by number, union clusterIds, never drops what a patch doesn't mention) and wire Operating Principle 0 + the culprit-PR mandate + the routing step in ai-tfa-coordinator.md to call them after any live gather. Confirmed on a real prior run (vrt_345): a representative's own deep PR-diff investigation was exactly the kind of work its sibling had no way to reuse under the read-only version of this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The write-back added in 70ef859 had every coordinator read-modify-writing one shared JSON file. Measured under 8 concurrent writers with a realistic read -> work -> write window, that design lost 28 of 40 updates (70%). Replace it with a layout where contention is structurally impossible instead of merely unlikely: the orchestrator solely owns the base file, and each coordinator writes only its own shard at rca-evidence.<buildId>.contrib/<testRunId>.json. No two processes ever open the same file for writing. readEvidenceFile folds base + all shards into one view (sorted order; real evidence beats a recorded gap; PRs unioned by number), and readBaseFile keeps the orchestrator's own write path from absorbing shard content back into base. Same 8-writer test against the new layout: 0 of 40 lost. A 12-process run hammering one repo key kept all 480 writes. A corrupt/half-written shard is skipped rather than breaking every subsequent read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two measured sources of duplicate work on a real 10-test build: 1. `gh` was 37% of all coordinator tool calls (151), and 46 were byte-identical commands re-run by different coordinators — one BStackAutomation spec fetched 12 times, a frontend component 5 times. None of these fit the evidence file's schema (deployState / prsInWindow), so the write-back added earlier could not share them. Add a build-scoped memo cache keyed by the CALL rather than by evidence shape, so duplication is caught regardless of what the call was for. `bin/cached-exec.mjs` wraps a shell fetch transparently (same stdout, same exit code, executes only on a miss); `bin/cached-mcp.mjs` does check-then- store for read-only MCP queries. One file per call key + atomic rename, so concurrent writers cannot collide or produce a torn read. Failures are never cached (a transient rate-limit must not become a permanent answer), stateful tools (tfaRcaTurn/getTfaTurnResult/triggerRcaReport) are refused, secrets are redacted before anything reaches disk, and the runner uses execFile with a hand-rolled tokenizer so no shell ever interprets an argument. 2. Drain reads plus their sleeps were 23% of all coordinator tool calls. The drain treated "TFA is still thinking" and "the TFA run hard-failed" identically, spending the full 40-read/10-min budget on turns that had already died — the four tests that wedged this way were the four slowest in the batch. Distinguish the two: stop after `maxErrorReads` (default 3) CONSECUTIVE hard failures, note it as `tfa-error`, keep the row resumable. A single good read clears the streak, so flaky-but-recovering reads still land. Measured: cross-writer cache hit served in 0.185s vs 1.131s for the live fetch. 108 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three on-disk artifacts live under a world-readable OS temp dir and carry sensitive content: the tool cache holds raw gh/kubectl output (private repo source, internal hostnames, log bodies), the evidence file holds PR detail and app-log digests, and the state CSV holds root causes and culprit PRs. Default umask left them 0644 — readable by any local user. Create directories 0700 and write files 0600. The file mode is the load-bearing control: a pre-existing directory keeps its own permissions (silently chmod'ing a caller-supplied stateDir would be presumptuous), but entries are 0600 either way, and a traversable cache dir only exposes opaque hash filenames. Note this hardens storage, not the redaction: redact() is best-effort pattern matching over token-shaped strings and was never sufficient on its own to make these files safe to leave world-readable. Verified on disk for both a fresh dir (0700/0600) and a pre-existing 0755 dir (files still 0600). 111 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dispatched three coordinators concurrently on a live build. They surfaced issues no unit test had, worst first: 1. CORRECTNESS: an empty `prsInWindow` with `gap: null` is byte-identical whether the PR search ran and found nothing or was never populated. Seen live — the file asserted 0 PRs for a repo that actually had 21 (the loader's search had silently returned empty), which would let a coordinator report "no culprit PR identified" with false confidence. Add an explicit `prsSearched` flag (sticky across contributors), a `hasTrustworthyPrList` helper, and a `coverage.reposWithUntrustedPrList` signal. Repo-level coverage semantics are unchanged — trustworthiness is reported alongside, not folded into, covered/gapped. 2. The runnable-guard scanned the raw command string, so it refused legitimate read-only calls: `;` inside a jq expression, `&` inside a quoted URL. Check whole argv TOKENS instead — post-tokenization a quoted metacharacter is inside an argument (harmless, we execFile) while a real operator is its own token. Verified both previously-refused commands now run. 3. execFileSync BOTH inherits and captures stderr, so relaying err.stderr printed failures three times. Capture only, relay once — wrapper stderr is now byte-identical to the command's own, plus one banner line. 4. Empty results were cached, making a sticky invisible negative. Not stored. 5. Nested single quotes made `--jq '...'` unpassable inside a quoted command argument. Accept `-` to read the command from stdin. 6. File mode applies on create only, so files left by a pre-hardening run kept 0644 forever. chmod explicitly on overwrite too. Also documented two traps that are usage, not bugs: `2>&1 | jq` merges the stderr banner into the pipe (the banner is correctly on stderr — one agent misreported this as a stdout bug; measured to confirm), and `cached-mcp get`'s exit code is lost behind a pipe. 118 tests pass, including regression tests for each fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replaying 223 recorded gh/kubectl calls from prior coordinator runs through the wrapper showed it was nearly useless in practice: only 25 calls were accepted and ZERO were hits. The cache was fine; the guard was wrong about what real traffic looks like. Three causes, each measured: - 89% of real calls embed the fetch inside a pipeline (`gh api X | jq .y`), which the guard refused outright. Now pipelines are parsed into a fetch plus a chain of pure text filters (jq/grep/head/... allowlisted), each run via execFile with no shell anywhere. Only the FETCH is keyed, so several agents filtering one fetch differently share a single network call. - `2>&1` appeared on 134 of 223 calls — agents add it reflexively because gh is chatty. It says nothing about what to fetch, and the wrapper captures stderr separately anyway, so it is normalized away instead of refused. Refusing it alone drove acceptance to zero. - `>/dev/null` was classified as "looks mutating", which is both wrong and misleading. Redirection is now separate from mutation, with an accurate message; genuine file redirects are still refused. Result on the same recorded traffic: 123/223 accepted, 32 duplicate fetches eliminated, 26% hit rate. The rest are `for repo in ...` shell loops, which legitimately need decomposing into one fetch per call — that is also better for cache granularity. Verified end-to-end that `gh api X 2>&1 | jq .a` and `... | jq .b` now share one cached fetch, and that chaining, non-filter binaries and file redirects are still refused. 123 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dispatched six coordinators concurrently against a warm cache. Five of six resolved; the swarm found three real defects, one of them data-destroying. 1. DATA LOSS: redaction consumed 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 214,074-byte response was cached as 816 bytes with the content field silently gone. Every private-repo file fetch through the cache was corrupted, and a coordinator hit exactly the predicted failure: it could not verify a line-number-specific hypothesis and nearly falsified a claim that was in fact TRUE (TestRunService.java:1747 really is an unguarded `.get(0)`). Redaction is now bounded by the first structural delimiter. Two already-corrupted entries were purged from the live cache. 2. Neither tokenize nor splitPipeline handled backslash escapes, so `\"` read as a closing quote. Two coordinators hit the two symptoms: jq string equality arrived mangled (`select(.filename==\a/b.json\)`), and a `|` in a jq regex alternation was split as a shell pipe, refusing the command. One fix, both symptoms — escapes are now POSIX-style (literal everywhere except inside single quotes). 3. Two findings corroborated independently by multiple coordinators, now in the contract: a drain error kills the TURN, not the THREAD — resubmitting on the same threadId resolves immediately, whereas the old reading ended PENDING and discarded a resolvable test; and the turn-2 wedge correlates with message size (~1400/~1350-char submits failed, a ~940-char retry landed) against a configured 1000-char cap. Cache went 33 -> 63 entries over the swarm with 7 cross-agent hits, all seven writer shards present and no collisions. 128 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…abulary `flip` accepted only the lowercase CSV states and returned a bare `false` for anything else — including `RESOLVED`, which is the exact value the RCA_OUTPUT contract tells every coordinator to emit. Callers that didn't check the return saw no difference between success and a no-op, so their rows stayed `pending` and looked un-run. This was not theoretical: a six-coordinator swarm appeared to "skip" the CSV flip entirely. A later coordinator diagnosed the real cause — they had almost certainly called flip with `RESOLVED` and been silently refused. Field names had the same trap: `thread_id`/`status` were dropped without comment by the COLUMNS guard, since the columns are `threadId`/`rca_done`. Accept and normalize the output-block vocabulary (RESOLVED->resolved, PENDING->pending-resume, case-insensitive) plus the field aliases, and make genuine rejections and dropped keys warn loudly instead of failing silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ce and hit counts Three findings from a warm-cache verification run. 1. RETRACTION. The contract told coordinators to keep turn messages under 1000 chars to avoid the TFA wedge, on the strength of a two-point correlation (~1400/~1350-char submits failing where a ~940-char retry landed). A later run refuted it: a 240-char message wedged exactly as a 1500-char one did. The cap stays — a tight digest is the contract — but it is no longer presented as a wedge cure, and a wedge must not be read as evidence the message was too long. The reliable response remains resubmit on the same thread. 2. Contributions were invisible to the obvious read. Coordinators are handed the base path and naturally `cat` it, which shows base only — one agent reported "2 repos" when the folded view had 5, including an 11-PR observability-api entry a sibling had contributed. Shards are what make concurrent write-back safe, so add `bin/evidence-show.mjs` to print the merged view (with --summary flagging repos whose PR list was never actually searched) rather than give up the layout. 3. Hit-rate was unmeasurable in normal use: callers silence tool chatter with `2>/dev/null`, which also discards the banner the metric depends on. Banners now tee to TOOLCACHE_LOG when set, so stderr can be suppressed and hits still counted. Warm-cache result that motivated this: cross-agent hits confirmed — one coordinator re-filtered a sibling's cached 76KB PR diff three ways for free, another reused two cached diffs it never fetched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The MCP cache had stored zero entries across every live run. The cause is structural: an agent's get -> call -> put costs three tool calls on a miss to save one later, so skipping it is correct for a one-off query, and every coordinator did. Pre-seeding inverts the economics. The orchestrator already runs these log sweeps in Step 4, so depositing each digest under the key a coordinator would compute makes the agent's `get` a single call that usually hits. Verified: after seeding four VictoriaLogs digests, a coordinator get returns HIT, and arg-order canonicalization means a differently-ordered query still matches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1. `flip` silently dropped `view_rca` and `turns_used` — both are in the RCA_OUTPUT contract but had no CSV column, and `view_rca` is the dashboard link the entire run exists to produce. Added as columns. 2. An MCP cache HIT requires reproducing the args exactly, and canonicalization normalizes key ORDER, not content — so a coordinator that guesses the logql/window/limit triple misses. One run burned four probe calls guessing to save two, making the cache a net loss on that pass. Added `cached-mcp <build> list`, which prints every cached query with its exact args to copy: one call instead of guessing. 3. MCP banners were not tee'd to TOOLCACHE_LOG, so shell and MCP hits could not be totalled from one file. Both now log there. Verification-run result for the record: 22 tool calls vs 81 on the same test cold (73% fewer), but the saving came overwhelmingly from the shared evidence file, not the call-level caches (1 shell hit of 8; MCP roughly a wash after the failed probes). The caches only pay on repeated identical lookups, and that run's second turn demanded a code path nobody had walked — a low hit rate there is the correct outcome, not a cache failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sely Both from a live run on a second build (ObservabilityApiLaneSuite, 189 failures across 9 clusters). - `evidence-show --prs [repo]` prints mergedAt | #num | title plus the build's started_at. That table is the highest-value falsification per byte: on this run it disqualified 11 of 22 candidate PRs on the window guard alone, before a single diff was fetched. Previously a coordinator had to pipe --repo's raw JSON through an ad-hoc node one-liner to build it. - `flip(testRunId, fields)` — dropping the leading csvPath — used to bind an object to testRunId, read a nonexistent CSV, and return a bare `false` that a caller mistook for success. It now says exactly what went wrong and shows the correct signature. Cache behaviour on this build, worth recording: the shell cache was COLD, yet the representative still logged 4 hits of 7 fetches — concurrently dispatched siblings populated it for each other, including the single most expensive call (`gh pr diff 16450`). And `cached-mcp list` alone avoided two grafana calls: the seeded digests were self-sufficient, so no `get` was needed at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ve absence A live run on a second build found the pre-fetch structurally blind to its own build's cause. An upstream outage ran 06:12:00-06:15:51 while the build's finished_at was 06:12:21, so a sweep scoped strictly to started_at..finished_at saw 21 seconds of a 4-minute outage. Step 4 now pads to started_at-2m .. finished_at+10m and labels findings inside/outside the strict window — without licensing an arbitrary window, which is the opposite failure (matching a coincidence in unrelated traffic). Two query mechanics added, both of which cost real calls in that run: - `direction` defaults to newest-first, so a limited query returns the END of the range. Verifying this finding required direction:"forward" — my own backward queries kept returning traffic clustered at each window's tail, which looked like the gap was absent when it was simply out of view. - Absence needs a control: a zero-result query is indistinguishable from a bad selector. Prove the logger was alive in the same window with a query you expect to be non-empty before treating silence as evidence. Verification note on the finding itself: the gap is real (zero /ext/v1 06:12:00->06:15:51, resuming with a 500 on /ext/v1/badge/build). Two secondary details in the coordinator's report did not hold — traffic resumes at 06:15:51 not 06:16:30, and badge traffic was not absent until 06:48. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng a negative Both from a live run on a second build. - `TFA agent run failed` (the wedge, size-independent, fix = resubmit on the same thread) and `turn expired or not found` (observed on a ~2000-char over-cap submit) are different failures. The latter's text names a thread/turn problem, so a coordinator reads it as a wedge and resubmits unchanged instead of shortening. Named both explicitly. - A coordinator nearly concluded a manifest lacked an entry when the fetch had been cut at ~64KB; its own `wc -l` control caught it (1042 lines vs 1518). Investigated and the tool cache is NOT the cause — verified 104KB and 214KB files round-trip intact, and the only truncation is past 256KB with an explicit marker — so this was surrounding tool plumbing. The hazard is real regardless of layer, and a silent truncation converts "grep found nothing" into a false negative, so: size-check a large fetch before treating an absent match as evidence of absence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured across three real runs (407 gh calls): per-PR `gh pr view <n> --json files` was 44 calls (10.8%), and every one is avoidable. `gh pr list` accepts `--json files`, so ONE request per repo returns every PR's changed paths — the Step 4 call we already make, just asking for one more field. Verified live on build fiehz…: 2 calls covered 22 PRs / 104 changed paths, replacing up to 22 per-PR fetches across coordinators. Path-overlap — the first falsification test in github-evidence.md — is now answerable from the evidence file with zero calls. It independently surfaces testhub#16450 -> project.js, the culprit a coordinator previously spent calls hunting. Two other measured wastes named in the same step: connector re-probes (`gh auth status` / `kubectl version`) were 17 calls (4.2%) purely because the manifest wasn't trusted, and file CONTENTS were 31% but only partly predictable — so those are deliberately NOT bulk-fetched. The `files` lists tell a coordinator which files matter; the tool cache dedupes the ones two coordinators both open. Diffs stay on-demand: large, and few PRs need one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
File CONTENTS were 126 of 407 gh calls (31%) across three real runs — the largest remaining slice. Six of the eight repos this workspace needs are already cloned locally, so most of those round-trips are avoidable. Measured: `gh api .../contents/<path>?ref=<sha>` ~1022ms vs `git show <sha>:<path>` ~37ms — 27x faster, byte-identical. A stale clone needs one targeted `git fetch` (~5.4s), which pays for itself after ~6 reads of that repo. SHA-PINNED ONLY, and this is the whole reason the module is careful rather than a one-liner. This workspace's clones were 12 commits behind: reading testPlan.js from the local BRANCH returned 281,061 bytes where the real head had 282,315. RCA reasons about what changed in a window, so silently reading different code produces a confident wrong answer. Pinned to the build-time sha — which Step 4 already records as deployState — content is byte-identical and staleness stops mattering, because a commit either exists locally or it provably doesn't. Two deliberate non-behaviours: the library never falls back to the network itself (it reports remote-needed and lets the edge decide, so a local answer can't be silently substituted), and a path genuinely absent at that commit is returned as an ANSWER rather than a fallback trigger. Remote reads still go through the tool cache, so a repo without a local clone behaves exactly as before. 137 tests pass, including a real two-commit git fixture that exercises the stale-branch-vs-pinned-sha distinction rather than mocking it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…literals The first cut of bin/repo-read.mjs defaulted RCA_WORKSPACE_ROOT to a specific developer's path and RCA_SHIPPING_BRANCH to observability_pre_prod. That is the exact coupling the capability-manifest design exists to prevent: which repos exist, where they are checked out, and what branch ships are facts the product CONNECTOR SKILL owns and the gate resolves, not something the plugin may assume. As written it would have worked for one product on one machine and silently misbehaved elsewhere. Both are now required inputs. Missing RCA_WORKSPACE_ROOT fails loudly with an explanation instead of guessing; the shipping branch is optional and only widens a fetch on a miss. lib/repo-source.mjs names no repo, branch or path — callers pass everything. Audited lib/, bin/, workflows/ and config/: zero product literals remain. 137 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…evidence file Two follow-ups to the local-repo reader. GENERIC DISCOVERY. discoverWorkspaceRoot() finds the clone directory without any hardcoded path: it tries the caller's dir, its parent, then grandparent, and accepts one only if it actually contains a repo from THIS run's validated list. That check is what makes it generic — a different product with a different layout resolves by the same rule, and nothing about repos, branches or paths is assumed. It is deliberately bounded to ~3 tries: climbing further risks matching an unrelated checkout, which is silently wrong rather than merely slow, and "not found" is a perfectly good answer since the caller falls back to the network. An explicit RCA_WORKSPACE_ROOT is honoured but still verified, so a stale override fails loudly instead of quietly. RESOLVED ONCE, NOT PER-AGENT. resolveLocalRepos() + setLocalRepos() record, in the evidence file, which repos are readable locally at their pinned shas. A coordinator reads that map and knows immediately whether to go local or remote, with no filesystem probing of its own — the same duplication the evidence file already removes for PRs and logs. Resolution order in bin/repo-read.mjs is now: evidence file, then RCA_WORKSPACE_ROOT, then bounded discovery, then network. Verified end to end on this workspace: gate discovered the root in 2 tries, resolved 3 of 4 repos as locally readable (the fourth has no clone and falls through to the cached gh path), and a coordinator then read a 145KB file with no network and no probing, sourced "via evidence-file (resolved at gate)". 144 tests pass. Not pushed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rompt The reader worked but nothing invoked it. bin/repo-read.mjs was referenced in zero skill/agent files, and discoverWorkspaceRoot/resolveLocalRepos/ setLocalRepos were called from nowhere outside tests — the whole local-clone path was unreachable at runtime, so every coordinator still went to the network for file contents (31% of gh traffic). Three changes close that: - SKILL.md Step 4 gains a step 6 that resolves the workspace once against the run's validated repo list and persists the per-repo map via setLocalRepos, pinned to deployState commit shas rather than branch names. - ai-tfa-coordinator.md documents repo-read alongside cached-exec/cached-mcp, with the sha-not-branch rule stated as the reason rather than a rule. - repo-read derives the evidence path from the buildId it already receives, so there is no RCA_EVIDENCE_FILE for a dispatch prompt to forget; an explicit override still wins. Verified with no env set: read teststack@41cc211b from the local clone with no network, sourced "via evidence-file (resolved at gate)"; a branch name is still refused. 144 tests pass. Not pushed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three fixes, one of which prevents the other two from recurring. REACHABILITY TEST (tests/wiring.test.mjs). Twice a helper shipped fully unit-tested and invoked by nothing. Unit tests cannot catch it — the module works perfectly alone, which is why it survives review. The prompt layer is the real call graph, so the test asserts every bin/*.mjs is named by some skill/agent markdown, plus that gate-critical lib exports have a call site outside tests. Verified it actually fires: adding an orphan file fails the suite with the helper named. FOLDED READS. Operating Principle 0 told coordinators to `Read` the evidence path — which shows the orchestrator's base file only and hides every contribution shard, the exact bug bin/evidence-show.mjs exists to fix and which cost a real run a re-gather of an 11-PR entry. It now directs --summary/--prs/--repo, and explains why reading the raw JSON is both wrong and more expensive. STALENESS. The file is keyed by buildId alone, so a resumed run silently reuses deployState and a PR window that have both moved on — the same silent-wrong-answer risk we refuse branch names over, with no guard at all. stalenessOf() reports age and advice; it deliberately does not expire anything, since stale build-level context still beats none and the failure window itself never moves. evidence-show warns on stderr on every view, so piped JSON stays clean. Caught while testing on the live build: a FUTURE timestamp clamped to age 0 and reported "fresh" — failing in the reassuring direction on clock skew. Now returns known:false, stale:true. 148 tests pass. Not pushed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing columns readRows keyed rows by whatever header the file had, while writeRows only ever emits COLUMNS — so any header name not in COLUMNS was silently discarded on the next write. Found on a real legacy 10-column state file (test_id,test_name,error_summary,is_flaky,...): flipping one row through the current code returned "resolved" and reported success, having dropped test_id and test_name entirely and blanked cluster_id. Losing which test a row describes is worse than any error we could raise. readRows now normalises the header through COLUMN_ALIASES (previously applied only to flip()'s field names, not to the header) and throws on anything still unrecognised, naming the offending columns and saying to re-seed. Known legacy spellings — test_run_id, status, thread_id, turn_id — keep working. This is why build fiehz... cannot simply be resumed: its CSV predates the current schema and must be re-seeded. 150 tests pass. Not pushed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mkdirSync's `mode` applies only when it creates the directory — the same trap already fixed for the files themselves. Found on this machine: <tmpdir>/bstack-rca was drwxr-xr-x, created before the hardening landed, with correctly-0600 files inside it. The directory listing alone leaks which builds were analysed, and pre-hardening files inside it are still 0644. These artifacts hold root causes, culprit PRs and log excerpts and live in a shared OS temp dir, so ensureOwnerOnlyDir() now chmods an existing directory to 0700 as well as creating new ones that way. Shared by csv-state, evidence-file and tool-cache, which all had the identical pattern. 151 tests pass. Not pushed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 again. On this machine that left 6 files at 0644 holding root causes, culprit PRs and log excerpts in a shared OS temp dir. Fixing it needs a sweep, not another per-write guard. hardenStateDir() walks the tree and makes it owner-only (0700 dirs / 0600 files). Idempotent and cheap, so Step 2 now runs it unconditionally. It never throws: a file owned by another user is skipped and counted, because failing an RCA run over one un-chmod-able leftover is worse than the leak it closes. pruneStateDir() addresses growth (2.2MB / 143 files / 6 builds accumulated here, nothing ever deleting them). It is deliberately NOT automatic and defaults to 7 days: these files ARE the resume state, keyed by buildId, so anything that deletes them can silently turn a resumable build into a lost one. dryRun reports without touching. Uses mtime, not atime, so reading a file during a resume doesn't make an abandoned build look fresh. Applied to the live directory: 5 dirs / 143 files swept, 6 leftovers repaired, 0 skipped; prune dry-run confirms nothing is old enough to remove. Two test bugs found and fixed while writing this: a hardcoded future nowMs made the whole fixture look ancient (passing for the wrong reason), and `new Date(seconds)` was read as milliseconds, landing every stamp in 1970. 155 tests pass. Not pushed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both only surface when the gate runs for real; neither is visible to unit tests.
CONNECTOR DISCOVERY MISSED EVERY CONNECTOR. Step 0 ran `ls .claude/skills/
~/.claude/skills/`. When the 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
the listing found nothing and the run would have degraded to raw MCP tools with
best-effort repo guesses, exactly the failure mode Step 0 exists to prevent. It
missed all three real connectors here (tra-regression-context, nl2steps-github,
nl2steps-infra). Now also checks ../ and ../../.
PINS EXISTED ONLY AS PROSE. resolveLocalRepos needs {repo: sha}, but Step 4
wrote the build-time sha into deployState.summary as English ("Branch tip on
<branch> at build start = cd88535b"). There was no structured field, so the
gate got an empty pin map and every repo fell back to the network while
appearing to work — 0 of 6 local instead of 3 of 6. deployShas() prefers an
explicit deployState.sha and falls back to parsing the summary, reporting which
happened. The fallback anchors on the build-start phrase: a bare hex word would
also match the deploy timestamp 260731135020Z sitting in the same sentence.
Verified end to end on build fiehz…: workspace found in 2 tries, 3 of 6 repos
resolved local, and a coordinator read a 280KB file with no network.
156 tests pass. Not pushed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both fold paths deduped prsInWindow with `String(p.pr)` as the key. When a contributor writes back PRs without a `pr` number, every one of them keys to the literal string "undefined" and the list silently collapses to whichever arrived last. Caught during a live dry run, not by a test: a coordinator reported writing a 6-PR BStackAutomation window for its 21 siblings; the file kept 1, with `pr: undefined`, and still flagged the entry `prsSearched: true` — so the gap read as closed while five PRs had vanished. Siblings would have inherited a confidently-wrong window. prKey() now falls back to url, then title, then a per-entry anonymous key, so unnumbered PRs stay distinct and two unknowns are never treated as the same PR. Numbered PRs still merge across writers, and '#10' and 10 normalise to the same key. 158 tests pass. Not pushed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uster_id
Root cause of the vrt_742 run's 30-minute wall clock, found by reading its
transcript rather than inferring it.
clusterRows() assigns cluster_id by MUTATING its input and returns
{rows, clusters}. The orchestrator ran:
const { clusters } = clusterRows(rows); // never writeRows(csvPath, rows)
so the clusters printed correctly, the run looked clustered, and every
cluster_id went to the CSV empty. Fan-out then had no representative/sibling
structure and dispatched one coordinator per test: 12 tests became 26
subagents and 30.5 minutes, even though per-agent cost was healthy at 12.2
calls (vs a 45.4 baseline). The savings were real but spent on agent count
instead of banked as time.
Two independent callers made this exact mistake on the same day — including me,
in the session that found it. That makes it an API footgun, not a user error,
so the fix is at the API: clusterAndPersist(csvPath, csvState) reads, clusters,
writes back, and throws if the persisted count doesn't match the row count.
Step 3 now mandates it and tells the reader to verify cluster_id is non-empty
before fan-out, since an unclustered run degrades silently to O(tests).
159 tests pass. Not pushed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…_seed
The sibling path DID run on the vrt_742 build — and inverted. Siblings averaged
22.7 tool calls and 2.2 turns against 8.0 and 2.0 for the representative they
were meant to be a cheap fraction of; one burned 60 calls over 17 minutes.
Cause: the coordinator has always accepted a `pre_seed` input, but Step 5 never
said siblings must be dispatched AFTER their representative lands, and never
said the dispatch prompt must carry the rep's finding. A "one-turn confirm"
with nothing to confirm degenerates into a full independent investigation with
the sibling framing on top — hence costing more, not less. Nothing detected it
because the RCAs were still correct; only the cost was wrong.
siblingPreSeed(csvPath, csvState, clusterId, representativeId) builds the seed
from the representative's landed row and returns {ok:false, reason} when the rep
is not resolved or recorded no root_cause — those siblings must not be
dispatched yet. The seed carries an explicit "confirm against your own evidence,
do not adopt this" instruction so the independence rule travels with it.
Step 5 now states the per-cluster barrier: rep first, wait, then siblings.
Clusters still run concurrently with each other; the barrier is per cluster.
160 tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Telling coordinators in the prompt to use evidence-show was not enough. Measured on the vrt_741 run: 21 of 25 evidence-file reads were raw cat/grep/Read against the base path and only 4 went through the folded view. A raw read shows the orchestrator's base ONLY and silently hides every contribution shard — precisely the representative-to-sibling context the file exists to carry, and the objective we were about to report as done. Three different agents each cat-ed the same file and each saw a partial picture. The file now says so in the first bytes anyone sees. JSON has no comments, so three `_`-prefixed marker keys are stamped first on every base write: what is missing, the exact evidence-show command to run instead, and why. They are stripped on read, so nothing downstream sees them as data, and re-stamping is idempotent rather than accumulating. Also checked what prompted this: the tool cache is barely used now because the redundancy it was built for is gone. vrt_741 made 169 shell calls of which only 6 were cross-agent repeats (4%), against the original "46 exact gh repeats for 10 tests". The MCP repeats are all same-agent getTfaTurnResult polling, which the cache correctly refuses as stateful. Low cache usage is the evidence file working, not the cache failing. 162 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…wRca semantics
Investigated two things I had reported as open bugs. Both were wrong, and the
investigation surfaced a real one underneath.
NOT A BUG — turnId empty on resolved rows. TFA returns turnId ONLY on a soft-
PENDING turn ({status, threadId, turnId}) and omits it on RESOLVED/NEEDS_INFO.
Verified across two runs: 8 of 41 and 12 of 38 tool results carried a turnId,
all of them PENDING. An empty turnId on a resolved row is correct.
NOT A BUG — viewRca being a generic hostname. TFA itself returns
"https://automation.browserstack.com — open the build's AI report (...)";
coordinators passed it through faithfully. The real per-build URL comes from
triggerRcaReport at Step 6, once per run, not per test.
THE REAL GAP. Because turnId exists only on PENDING — exactly the case that
produces pending-resume — a row can land resumable with no turnId, and then the
resume path cannot drain the in-flight turn: it submits blind onto a thread that
still has a turn running. Such a row is indistinguishable from a healthy one in
the CSV. flip() now warns loudly, naming the testRunId and what to do. A warning
rather than a rejection: losing the row entirely is worse than resuming
imperfectly.
Coordinator doc now states both semantics so the next agent doesn't re-derive
them or invent a more specific link than the data supports.
Also removed four stray log files left in the working tree.
163 tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… drifting The last two runs got slower and the cause was self-inflicted. On vrt_740, 92 of 407 tool calls (23%, 5.1 per test) were agents re-deriving the plugin's own API at runtime — `grep -n "^export function" lib/…`, `cat config/…`, repeated `ls .claude/skills/`. The previous run spent 13 (5%, 0.9 per test). That is ~79 of the ~120 extra calls between the runs, and most of the wall-clock regression from 18.2 to 36.3 minutes. Nothing was wrong with the helpers. Five were added this session to fix five silent correctness bugs, each justified — but the SKILL named them without signatures, so the only way for an agent to learn a call shape was to read the source. The plugin was taxing every agent to learn itself. Adds one "API reference" section before Step 0 with every signature a run needs, grouped by module, plus the bin/ commands, the three load-bearing constants (COLUMNS, RESUMABLE, TEST_LOGS) and the config keys. Fully generic: build ids, repos, branches, workloads and paths are all inputs supplied by the gate and the connector skills — verified no product or machine literals. Documenting it fixes today; drift is what actually caused this, so a test now asserts every exported lib helper appears in that section, with an explicit INTERNAL allowlist for module internals agents reach through bin/ instead. Verified it fires: adding an undocumented export fails the suite by name. For the record, this run also confirmed the fixes that preceded it: the base file's partial-view markers moved evidence reads from 16% to 74% folded (4:21 to 43:15), clustering persisted 18/18, the sibling barrier held with 0 duplicate dispatches, and gh calls per test fell 4.4 to 4.1 — so the hygiene fix displaced live gathering rather than adding to it. 164 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng it Swept for the two bug classes this session kept producing — mutate-and-return footguns, and hard rules that live only in the prompt. Class 1 (mutate-and-return) came back clean: clusterRows is the only one, and clusterAndPersist already covers it. A pendingRows hit was a false positive in my own grep, not a defect. Class 2 turned up "A PRODUCT_BUG RCA without a culprit PR is incomplete" — load-bearing, and enforced by nothing. Checked it against four real builds before changing anything: it has held. Of 9 product-bug rows, 3 carried a PR link and 6 carried an explicit "none — searched <what>", which the rule permits. Zero were blank. So this is a guard, not a bug fix. Worth adding anyway: a blank related_prs is indistinguishable in the CSV from a genuine dead end, so the failure would be a silent product-bug attribution with no evidence trail — the same shape as every other bug found this session. flip() now warns when failure_type is a product bug and related_prs is EMPTY. A stated "none, searched X" is compliant and deliberately not nagged. 165 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y with Step 4, and gather NEEDS_INFO asks concurrently Step 4b submits each cluster representative's turn 1 directly (tfaRcaTurn), in the same tool-call batch as Step 4's evidence pre-fetch, instead of waiting for Step 5's coordinator dispatch to submit it. A RESOLVED turn 1 flips the CSV row straight to terminal and dispatches its siblings immediately, with no Step 5 dispatch at all for that representative; a NEEDS_INFO/PENDING outcome is handed to the eventual coordinator via the new turn1_result/resume inputs (lib/turn1-registry.mjs) so turn 1 is never resubmitted. Wired through all three execution paths: lib/loop.mjs (sequential harness), workflows/rca-batch.mjs (Workflow path, plus a resume/turn1_result mutual-exclusivity guard so a prior-run pending-resume thread always wins over a same-run Step 4b entry), and agents/ai-tfa-coordinator.md (direct-dispatch prose contract). Also parallelizes a NEEDS_INFO turn's independent gather() calls (lib/routing.mjs's routeAsk/routeAsks classify each ask with no cross-ask state — priority only orders the assembled message, never the fetching) via Promise.all instead of one round-trip at a time, mirrored in the coordinator's own prose contract. Coordinator paths (references/evidence-routing.md, references/github-evidence.md) are also now consistently pluginRoot-qualified rather than bare, so a dispatched coordinator never resolves them against the wrong cwd.
…l report cleanupBuildArtifacts(buildId, stateDir) removes this build's own CSV, evidence file + .contrib shards, tool cache dir, and turn1 registry — called only after triggerRcaReport succeeds (Step 6), never a periodic sweep. Safe specifically because Step 6 only runs once every row is terminal: there is nothing left to resume for this build at that point. Deliberately separate from lib/state-dir.mjs's pruneStateDir, which remains the age-based safety net for a build that crashes before ever reaching Step 6.
…ver client-side signatures
clustersFromThemes(rows, themesResult, testsByThemeId) builds { rows, clusters }
from getBuildFailureThemes + listTestsInFailureTheme (server-computed themes),
mirroring lib/signature.mjs's clusterRows() shape so downstream fan-out code
doesn't care which path produced it. Falls back to lib/signature.mjs's
client-side text-signature clustering when getBuildFailureThemes reports
ready: false. A row already claimed by an earlier theme is skipped
(first-theme-wins) rather than landing in two clusters; any failed test the
server didn't assign to a theme becomes its own singleton cluster, never
silently dropped.
…h, and Step 5/6 wiring Two bodies of work landed together in this file (interleaved at the hunk level, so split by content below rather than by commit): - Step 3: document the server-side clustersFromThemes path (getBuildFailureThemes + listTestsInFailureTheme) as preferred over lib/signature.mjs's client-side clustering, with the fallback condition and verification step. - Step 4b (new): turn-1 pre-dispatch for cluster representatives, concurrent with Step 4's evidence pre-fetch — mechanic, the three-way branch on RESOLVED/NEEDS_INFO/PENDING, and the skip condition for a representative already in pending-resume state. - Step 5: wires the turn1 registry into each representative's dispatch (resume for PENDING, turn1_result for NEEDS_INFO); rewrites the "per cluster, not global" sibling-ordering guidance into an explicit rolling work-queue, distinguishing the Workflow path's true per-cluster streaming from the direct-Agent-tool-dispatch path's per-batch limit; adds an explicit warning that the direct-dispatch path has no code enforcing the Step 4b handoff, unlike the other two execution paths. - Step 6: calls cleanupBuildArtifacts only after triggerRcaReport succeeds. - Hard rules: carves out Step 4b's direct tfaRcaTurn call as the sole, narrow exception to "always dispatch via ai-tfa-coordinator". - API reference: documents lib/turn1-registry.mjs and lib/build-cleanup.mjs's exports (enforced by tests/wiring.test.mjs's doc-drift guard).
…ize Step 1 connector probes Step 4b was previously documented as running "concurrently with Step 4" via a same-batch trick, but a real run executed it sequentially anyway because the wording didn't specify which of Step 4's several batches to merge into. Rewrite it around the Agent tool's genuine fire-and-forget semantics instead: dispatch one lightweight subagent per cluster representative for turn 1, proceed to Step 4 in the very next turn without waiting, and handle each result as pure bookkeeping as notifications interleave with Step 4's own turns. Adds: - TURN1_OUTPUT fixed-shape contract so concurrent dispatches stay attributable - concurrency-capped dispatch (reusing config's existing `concurrency` value) - fail-open documentation for a subagent that never reports back - immediate sibling pre-dispatch when a representative resolves in one turn - explicit "narrate as one combined phase" guidance to stop the same bug recurring via mis-narration Also applies the same independent/parallel-batching fix to Step 1 Gate Part A's connector base probes and scope probes, which had the same ambiguous sequential-looking wording. Pressure-tested via superpowers:writing-skills (fresh subagents given only the wording, no other context): Step 1 and two of three Step 4b scenarios executed correctly end to end. One gap found — an agent could still insert an unnecessary Step-4b-only turn before Step 4's own tool calls began, since nothing said Step 4b's prep (registry init, resume skip-list) has zero dependency on Step 4. Closed with an explicit sentence permitting/encouraging both in the same first turn.
Pressure-tested every pre-today section of SKILL.md the same way as this
session's Step 4b/Step 1 rewrite (superpowers:writing-skills, fresh subagents,
verbatim section only). Step 3 (clustering path selection), Step 5
(rolling-refill work-queue, sibling guard), and Step 6 (cleanup-after-report
gating) all executed correctly across every branch tested.
One gap in Step 5: its own bullet only cross-referenced "resume/turn1_result
per agents/ai-tfa-coordinator.md" without restating which registry status maps
to which field. A fresh agent given only Step 5's text folded a NEEDS_INFO
result into `resume: true` instead of the documented `turn1_result:
{threadId, asks}` shape — the exact mapping only lived in Step 4b's own
closing paragraph, which didn't survive being read in isolation. Restate the
mapping explicitly in Step 5's own bullet so it doesn't depend on carrying
detail forward from an earlier section.
…tative rule Every "measured: X of Y (Z%)" / "A vs B" citation was a one-time measurement from a specific past run, not a guarantee about the next one — the digits added token cost and false precision without changing the instruction. Replace each with a plain statement of the failure mode and, where it matters, a brief "this has happened on a real run" flag. The rule and its consequence are the load-bearing part; the exact arithmetic was decoration.
…ce pre-fetch, and coordinator probes Adds <use_parallel_tool_calls> guidance (per Anthropic's parallel-tool-use docs) to SKILL.md and ai-tfa-coordinator.md, bulletproofs Step 1's gate-probe batching rule (a real run violated the existing rule anyway - 4+ min cost), adds the same batching contract to Step 4's per-repo/per-workload pre-fetch loops and to github-evidence.md's per-probe github hunt, and adds tables of contents to evidence-routing.md/github-evidence.md (both >100 lines) per the agent-skills best-practices doc.
A forensic pass on a real run (build hazbi3bs...) found four documented steps were skipped entirely, not just executed out of order: - Step 1A: only one of three present connector families was ever opened (a11y-regression-context, the one matching the build's actual failures, was never read) - adds a required enumerate-then-match check. - Step 1B: two separate AskUserQuestion calls instead of one, for fields the selected connector's own intake-defaults section already answered outright - adds a check-the-connector-first rule and bulletproofs "never a second question" with a STOP condition. - Step 1A scope probes: zero scope-probe calls ran despite the connector declaring seven - adds a required pre-Step-2 checkpoint. - Step 3: clusterAndPersist fallback ran without ever attempting the preferred getBuildFailureThemes call first - adds a required-order gate. - Step 5: fired the full fan-out with Step 4b (turn-1 pre-dispatch) never having run at all, so every cluster paid full coordinator cost with none of the one-turn resolve-and-skip fast path attempted - adds a required Step-4b-happened gate before Step 5's first dispatch.
The prior wording ("Step 4b must have already happened", "go back and do
Step 4b first") could be misread as waiting for Step 4b's subagents to
finish before Step 5 starts - reintroducing the sequential-latency bug Step
4b exists to remove. Reworded to be explicit: the gate only checks that the
dispatch batch was issued (fire-and-forget), same contract Step 4b already
documents, not that it completed.
…a stale variable Traced a real "test id mismatch -> falls back to client-side clustering" report to actual data: getBuildFailureThemes/listTestsInFailureTheme both returned correct, matching numeric testRunIds - no format bug. But the run's final CSV carried clusterAndPersist's c-xxxxx signature-hash IDs, not clustersFromThemes' theme-<id>/solo-<id> format, meaning the preferred path was silently abandoned. Root cause: an earlier listTestIds(status=failed) call had errored once and a later call used a different status filter in the same session - clustersFromThemes was fed whatever `rows` variable was still in scope, so every theme member came back unmatched (looks exactly like an ID mismatch, isn't one). Pins rows to readRows(csvPath) - the one row set guaranteed fresh and from a successful seed.
…not turnCap
Confirmed via real production logs (test-failure-agent-chat, namespace
testcasegeneration): a thread that gets "TFA agent run failed" twice in a
row is hitting the backend's openai.BadRequestError context_length_exceeded,
not a transient wedge - the thread's accumulated history exceeded the
model's context window, which a resubmit cannot fix since history doesn't
shrink. Rule 4b previously advised resubmitting all the way to turnCap,
wasting every remaining turn on a thread that structurally cannot recover.
Now: one retry for a first failure (still handles genuine transient wedges),
but two consecutive same-thread failures end the test PENDING immediately
with a new distinct note ("likely-context-exceeded"), added as a 4th
documented status alongside turn-cap/soft-pending/blocked.
…thread restart, not just a faster PENDING The prior fix (end PENDING after 2 consecutive same-thread wedges) only failed cleaner - it didn't get the test to an actual resolution, which is the real goal. Since the dead thread's oversized accumulated history is the only broken part (not the test itself), add a real recovery: on confirmed context_length_exceeded (2 consecutive same-thread failures), distill everything gathered so far into one condensed hypothesis message and submit it as turn 1 of a brand new thread - a single, narrow, justified exception to "never start a second thread" in step 4, since the first thread is provably dead rather than merely inconvenient. Bounded to exactly one restart per test; a restarted thread that also overflows twice ends PENDING for real rather than restarting indefinitely.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.