Feat/rca interview flow - #8
Open
ruturaj-browserstack wants to merge 10 commits into
Open
Conversation
…g test `ci` routed to the `github` capability, so a team whose CI system is not their git forge had no way to declare one. It is its own capability now. Flipping it naively would have been a silent regression: for the many teams whose CI *is* their forge, every `ci` ask would degrade to `action: "gap"` and `unavailableCapabilities` would declare `ci` missing to TFA on turn one, with nothing actually wrong. So `evidenceRouting.ci` carries `fallbackCapability: "github"`, resolved in `buildManifest` rather than in `routeAsk` — both read the manifest, and resolving it in the router would have left `unavailableCapabilities` reporting a gap the router then quietly served. The fallback target is looked up in `discovered`, never in the manifest being built, so a single hop is structural: `a -> b -> c` cannot resolve and no cycle is possible. There is deliberately no `fb === cap` guard — reaching that line means `cap` was not discovered, so the target lookup is empty and the existing guard already returns. I wrote that check and a test for it, found the test could not be made to fail, and removed both rather than ship a fifth vacuous guard. `discoveryHints` is deleted. It was a list of vendor names in config, copied into `routeAsk`'s gap payload, and read by nothing but `routeAsk`'s own test — so it taught a default while informing no decision. Adds `tests/config.test.mjs`, because **no test loaded the shipped config at all**. That is why the regression above would have shipped green: fixture tests prove the routing logic, and nothing proved the logic was wired to the file we ship. It asserts the ci-is-the-forge case against the real config, that every fallback names a capability that exists, that no entry carries a hint list, and that no capability name is a product name. Also adds `context.staleAfterDays`, which `lib/rca-context.mjs` will read. 191 tests pass, up from 176. Every new assertion proven by mutation: reverting `ci` to `github` fails two, reintroducing `discoveryHints` fails one, deleting the fallback pass fails two, and resolving the fallback from the manifest instead of `discovered` fails the chaining test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`lib/coverage.mjs` computed a coverage-capped confidence band. Nothing called it. Its own test was deleted earlier on this branch and the module was left behind, and both its exports sat in `tests/wiring.test.mjs`'s INTERNAL allowlist — so they were exempted from the documentation guard rather than wired to anything. Three references made it look alive, and none of them was one: - `lib/evidence-file.mjs` has a `coverage` field, but that is a different thing entirely (which requested repos and workloads have evidence). - `lib/csv-state.mjs` declares a `coverage` CSV column with **no writer anywhere**. - `agents/ai-tfa-coordinator.md` stated, in the present tense, that "`asks_unavailable` is the evidence-coverage signal the coverage stamp turns into a confidence band" — describing a mechanism that did not exist. A coordinator reading that would look for a stamp to produce and find nothing. The module is gone and that sentence now says what is actually true: record `asks_unavailable` accurately, the dashboard weighs it, there is no local stamp. The CSV `coverage` column STAYS. `readRows` throws on a foreign header, so dropping a column breaks resume for any build already in flight — the same reason `is_flaky` was never added. An unwritten column is inert; a changed header is not. Kept the idea in this message rather than in the tree: a RESOLVED RCA built with infra, logs and metrics all unavailable should not read like one built on full evidence. That is worth building. It was not built, and a module that only looked built was making it harder to notice. 191 tests still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The run assumed its way in. Part A enumerated connectors against a probe table naming six products; Part B assumed intake; the gate asked at most one question. Right shape for the repeat loop, wrong shape for first contact: a customer with nothing on disk got no explanation of what the plugin already has, and was never asked for the repos and branches the culprit-PR hunt depends on. Step 0 now loads .rca-context.json and branches on it. Step 0a states the ownership split. Step 0b is the interview. Both are lettered sub-steps because Step 4/4b are referenced by name from ten files outside this one, so renumbering was off the table. **The lifecycle boundary is a file, not a judgement.** "Am I still in first contact?" is exactly as unfalsifiable as the rule it would replace, and the history says the unfalsifiable version loses — 164962f added 52 lines enforcing a rule and 395960c added 82 more because it was violated again. So a new § The question budget states the rule ONCE, keyed on whether a resolvable profile carries a verified GitHub connector, with every other mention pointing at it. Budgets: 8 in first contact (stated as arithmetic), 1 at the gate, 0 after it closes. The all-caps "AFTER THE GATE CLOSES..." invariant is untouched — it is correct under the new design. The paragraph that most needed changing is the emphatic "no second gate question, ever": it now says explicitly that it governs the gate only, or an agent reads it as a prohibition on interviewing and refuses. Part A stops discovering and starts REPLAYING: re-run each capability's stored verifiedBy read in one batch. That reframing is what removes the probe table — the probe is data the interview wrote, not prose this file carries. GitHub becomes mandatory at gate time (it was "never a blocker"), while staying a gap mid-run, because a coordinator that refused would sink the batch. Part B's precedence now puts the persisted profile ABOVE connector intake-defaults. Without that, a connector skill's lane table overrides the answers a customer gave and a live read proved, and first contact proves nothing. Also fixes prose that was actively wrong: - SKILL.md's one-gate framing was MANGLED MARKDOWN. "(Step" then "1. before execution" rendered as a broken ordered list, so the file's most load-bearing sentence was not a sentence. - The vendor default survived in the two files that teach hardest: the gate template printed `via <kubectl ctx …, docker, ecs, …>` and the worked example showed `via kubectl`. De-vendoring SKILL.md while leaving those is de-vendoring nothing. Also cleared from evidence-routing.md, github-evidence.md's projection table, the coordinator, and README. config's $comment asserted "No kubectl literals" while containing one. - Headless support is removed entirely (6 sites), collapsing every interactive-vs-headless fork into one path — which is also what lets the interview always ask. kubectlSweep/victorialogs stay in setLogsEvidence and are now annotated as grandfathered schema names: renaming them breaks resume for in-flight builds. Tests: 190 pass. The one failure is tests/wiring.test.mjs demanding api.md document lib/rca-context.mjs, which is untracked work-in-progress from a concurrent unit and is NOT part of this commit — this tree checked out clean is green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds lib/rca-context.mjs (1358), bin/rca-context.mjs (262) and 82 tests, plus the
three references Step 0b delegates to and three guards over the whole thing.
**Two predicates, both decidable from the file, each with exactly one reader.**
`isRunnable` gates the run: github present AND its `verifiedBy` carrying a `count`
or an `observedAt`. Deliberately not "verifiedBy is non-empty" — that is a presence
check `{note: "TODO"}` satisfies, and an agent hedging instead of failing writes
exactly that. It is the defect this project already shipped once as a `checkedBy`
field holding a version banner, and it would have sat under the whole lifecycle
boundary. `isProvisioned` gates something different: whether the interview
finished. Runnable-but-unprovisioned is the customer who stopped after github, and
the gate offers to resume them rather than locking them into a github-only setup.
**`missingCapabilities` treats a fallback as coverage, and `ci` is why.** A team
whose CI is their git forge has no second system to record, so `ci` gets no
connector — and without this the only route to provisioned was recording a GAP on a
capability `buildManifest`'s fallback was demonstrably serving. The gate would then
offer to resume a finished interview on every run, or print "ci unavailable" about
a working connector. Single hop, read from config, so no hardcoded `ci` exception.
**Added `recordWarning`/`record-warning`, which the plan forgot.** The gate is told
to print `warnings`; nothing could write one after the first document existed, so
the empty-PR-window warning would freeze at first contact and any later warning
needed hand-editing a committed file — the thing this CLI exists to prevent. Shares
`recordGap`'s schema and one implementation, because a warning IS a gap that does
not degrade evidence; a test asserts a warning never satisfies `isProvisioned`.
**No credential detector, by decision, so the schema carries it instead.** Closed
objects: every object refuses a key it does not define, which is what makes "there
is nowhere for a secret to live" true rather than aspirational — remove the
allowlist and `{kind, name, value: "<secret>"}` persists. `scope` is the one
open-keyed object and a test asserts that hole exists deliberately rather than
pretending otherwise. Credential handling is prose in interview.md, and covers
every field the agent authors — `howToQuery.args`, `scope`, `verifiedBy.note` — not
just `credential`, because plenty of tools authenticate by query-string.
No regex anywhere in the module: matching is startsWith/endsWith, and validation is
character-class arithmetic. Not hardened to 0600 either — git preserves neither, so
it would be a confusing artifact; the test asserts the absence of `chmodSync`.
**Three new guards, each mutation-proven:**
- no vendor name on the new surface, plus templates/ and examples/. Three
pre-existing files grandfathered BY NAME with a reason each, so the exemption is
a closed list. It first reported a vendor inside `execSync` — "ecs" as a
substring — so it tokenises now.
- any file asserting a never-ask rule must point at § The question budget.
Unqualified, that rule reads as a prohibition on interviewing and an agent obeys
it. A per-file literal-phrase check, not a regex over English.
- every CLI verb named in a prompt file must exist. Written because it had already
happened: I put `rca-context.mjs read --build-name` and `rca-context.mjs upsert`
into SKILL.md and neither exists. Its blind spot is stated — it checks the verb,
not the flags, so the `read --build-name` half stays a review concern.
Subagent-reported, verified, and worth knowing: 68 mutations across the module all
killed their targets, and the sweep found two of the agent's own tests vacuous —
one used a subdirectory whose basename never matched `homeRepo`, so the mutation
was a no-op. `count: 0` is verified, not failed, because §6 classes an empty PR
window as a warning; the cost is that a deleted target looks like a quiet window
until something uses it, and the gate digest is the only place a human catches it.
276 tests pass, up from 191.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The destination was resolved through the `homeRepo` the document declared: search every level up PLUS each level's children, match a candidate by directory basename or `origin` remote, gate adoption on git-tracked-ness, refuse when nothing matched. ~90 lines answering "which repo owns this context". The proving run showed why that is the wrong question. The agent was invoked in a workspace folder holding three clones — not itself a repo — and the resolver reached sideways and wrote into `misc-services/`, a sibling the customer never named. It worked, and it was unpredictable: nobody could say where the file would land before it landed. The answer is now the directory you were invoked in. Nothing else. It need not be a git repo, because a workspace folder holding clones is a normal place to work. Deleted: `contextHomeDir` (46 lines), `candidateDirs`, `worktreeRoot`, `isTracked`, `originNames`, `homeRepoMatches`, and the whole adoption ladder. `trust` collapses from `own-worktree | origin-match | tracked | caller-supplied` to `cwd | ancestor | caller-supplied`. A bounded three-level walk UP survives, so running from a subdirectory of the same project still finds the file. `homeRepo` leaves REQUIRED_FIELDS. It was required because it selected the destination; nothing reads it now, and `repos.product` already carries the same information for code. Kept as an allowed optional field so existing contexts stay valid and a human opening the file still gets the line — but a required field that decides nothing is a question the interview asks for no reason. **A real bug the change introduced, caught by a test rather than by review:** the gitignore refusal ran `git check-ignore` unconditionally, which exits 128 outside a repository. That was reported as "could not determine whether this is ignored" and refused the write — so a plain directory, the exact case this commit exists to support, was rejected. It now establishes `--is-inside-work-tree` first and only then asks about ignore rules; a 128 from the second call is still a real failure. Also fixed: `locateContext` returned `forbidden: true` for a context sitting in the plugin's own checkout and `readRcaContext` ignored the flag, so it would have read it. That is now `plugin-root-context`, refused rather than read — the one directory cwd is never the right answer for, since the documented install flow leaves cwd there and a context written there stages the customer's scope into our repository. **What this gives up, recorded as a decision rather than an accident:** a directory is not necessarily a repo, so the file is no longer guaranteed committable and a teammate no longer inherits it by cloning. That is Confluence #11, team-shareable context, and it is now conditional on the customer working inside a repo and committing the file. Worth noting the proving run did not commit it either, even though it landed in a repo. 275 tests pass. Three mutations proven: resolving through a parent instead of cwd fails 16, dropping the plugin-root refusal fails 1, dropping the upward walk fails 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…capability
The interview searched for MCP servers and CLIs and never really searched for
skills. Two mentions existed — "enumerate connector-shaped skills" at T2 and
".claude/skills/" in the evidence hierarchy — with no paths, no glob, and nothing to
do with a hit. The proving run made zero calls touching `.claude/skills`, which is
what an unactionable instruction looks like from the outside. That workspace had
none, so the outcome was identical and nobody would have noticed.
T2 now names the four locations (project, workspace, one above, personal) and says
what a hit is FOR. A skill is not a hint: it carries the repo map, the branch
conventions and the query conventions its author wrote down, which is exactly the
knowledge that makes attribution accurate and that no probe can recover. So its
scope is taken as pre-filled and confirmed rather than asked — but still verified
with a live read, because a declaration is not evidence, and treating one as proof
is the defect that made the old gate trust scope probes it never ran.
**No discovery script, deliberately.** Globbing four paths and judging whether a
skill is about your capability is reading and judgement. A module for it would be a
list of places and patterns that goes stale — the thing this branch has spent four
commits deleting.
Storage is the other half, and that IS code's job: `connectors.<cap>.source` =
`{kind: "skill"|"mcp"|"cli"|"api", path?}`. Without it the interview could read a
customer's skill and then lose the fact that it had — a later run could not re-read
it, notice it changed, or tell a coordinator to follow it. `via` is free text, so a
metrics MCP server and a skill named after the same backend were indistinguishable.
`path` is REQUIRED for a skill and REFUSED for everything else. Required because a
record saying "a skill informed this" with no way back to the file is worse than no
record; refused elsewhere because `via` already names an mcp/cli/api and a second
name would only drift. Optional overall, so a connector the agent could not classify
stays unmarked rather than guessed, and contexts written before this field stay valid.
Also fixes prose I made stale in f45dec8 and did not update: seven references in
`interview.md` still described the destination as "the worktree root of the repo the
document declares as `homeRepo`" and cited `no-git-worktree`, both removed when the
context became anchored to the invocation directory. The CLI-verb guard could not
catch it — the verbs were right, only the meaning of `--from` changed.
279 tests pass. Mutations: dropping the skill-path requirement fails 2; a vendor
name in a comment I wrote was caught by the module's own no-default-stack guard.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lready grew Two halves of one problem. **The offer.** Hitting the question ceiling meant every unanswered capability silently became a gap. A customer willing to spend two more questions to make attribution better had no way to say so. T8 already asks a question, so this costs nothing extra: it gains a third option. Framed by VALUE, not by count. "Want to answer more questions?" asks someone to price something they cannot see; "`metrics` is a gap — 2 questions and pressure-vs-functional becomes distinguishable on this build" is a decision they can make. The label is built from the digest's own gap lines, and when nothing is closable the option is absent rather than offered as a bare "anything else?". It is also where a dropped `Something else` goes. When four real candidates fill T5's render budget the free-form entry is pushed out, so the open "anything else do you have?" — the one question that catches a stack nobody wrote down — never gets asked at all. Offering it by name here recovers it. **The bound, which matters more.** `interview.md` claimed a correction "never becomes a ninth question". That was false: re-asking the confirm IS another `AskUserQuestion`, and the proving run spent three of its five questions there. So the loop was unbounded while documented as bounded — and an opt-in extension on top of an unbounded loop is unbounded twice. T8 is now entered at most three times. On the third entry the extension option is gone, leaving only write/correct/discard, so it terminates by construction rather than by the agent's judgement. The honest worst case is 10, stated as arithmetic in § The question budget: 8 plus two further passes. Keeping it computable is the whole reason the ceiling is a number — a soft ceiling is the unfalsifiable prose rule this design replaced. A customer who wants to go further re-runs `/rca-build` rather than answering a fourth round: every capability persists the moment it verifies, so it resumes at the first one with neither a connector nor a gap. 279 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The proving run did emit the ownership split, and completely — what BrowserStack has, what only the customer can supply, GitHub as the one blocker, plus a concrete list of what it could see. The customer still experienced it as missing, and they were right to. It arrived SEVENTH, after five tool calls, quoted inside a status update that opened "No context file anywhere near either the plugin root or the working directory", and closed by promising "session inventory and write-target resolution". Buried in `ls` and `cat` output, framed as a footnote to a diagnostic, in vocabulary that means nothing to a customer. Three causes, and the last is the one that matters: 1. **The document ordered a tool call before it.** Step 0 said parse args → load context → outcome table, and only then Step 0a. An agent reading top-down does exactly that. Logically the context load must come first — you cannot know it is first contact until you look — but nothing said to look *quietly*. It does now. 2. **"Say what each side owns, before asking anything"** was satisfied literally: the first question came later still. It never said the greeting is the first thing the customer READS. It does now. 3. **The greeting is the only step in this flow with no observable artifact.** Every other step produces something that can refuse or be counted — a CLI call, a written file, an AskUserQuestion, a digest. This one produces prose, so the budget arithmetic, the ledger and the test suite are all blind to it. That is why it degraded to a remark between two Bash calls while every enforced rule held. A test cannot check what an agent says, so it checks the two instructions whose absence caused this: that the context load is explicitly silent, and that the greeting is framed as the first OUTPUT rather than merely before the first question. Removing either fails it. 280 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`source.path` was required for a skill and its FORM was unspecified. Three of the four glob locations produce paths that are not portable — `../.claude/skills` assumes the same workspace layout and `~/.claude/skills` is one person's home — and the context file is shared, so an unqualified rule silently ships a path that resolves for its author and nobody else. Relative to the context file when the skill is inside its tree; that is the portable case and a teammate cloning gets the same skill at the same place. Outside it, record it as read and say so at T8: re-verification runs on the machine where it resolves, so a local path is genuinely useful there, and an unresolvable one degrades to a targeted re-ask for that one capability rather than an error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eated One run left 28 files and 572 KB in the customer's repo root — fetched sources, saved diffs, raw API responses, redirected stderr, a drafted message. Litter was the lesser half: several coordinators had independently chosen the same short filenames, so they were overwriting each other's work. Nothing referenced any of it; the findings live in the CSV rows, the evidence shards and the dashboard report. `scratchDirFor(buildId, writerId, stateDir)` gives each agent a directory of its own, keyed on its writerId, under the state tree beside the CSV and the tool cache. Two properties, both structural rather than remembered: no two agents can collide however they name a file inside it, and nothing lands in anyone's repo. Owner-only (0700), because scratch holds fetched source and raw API responses — the same material the shards hold. Both id segments are sanitised, so a buildId from an argument or a writerId from a testRunId cannot escape the tree. Containment is not absolution: each agent still deletes what **it** created, by name, before finishing. Never a glob, never a sweep — `54d5bb0` removed `pruneStateDir` because the plugin runs on a user's machine and must not delete their data, and a wildcard is that hazard wearing a helpful hat. Only the agent that wrote a path can safely remove it, which is also why this cannot be delegated to the orchestrator or bolted on later. Two earlier attempts were the wrong shape and are worth recording. "Never redirect stderr to a file" was a prohibition, and coordinators sometimes genuinely need a file on disk — it would have been violated the same way. Then the guidance itself named `e1.log`, `s1.json` and specific fetched sources as examples, which teaches exactly those names; every concrete filename is now gone from the prose, for the same reason the vendor lists went. Guarded: dropping the section, the helper reference, the by-name scoping, or the never-deletes-what-it-did-not-create guarantee each fails. Mutations proven on the helper too — removing writerId from the path makes two agents collide, and removing the sanitiser lets a segment escape. Also: `--stats` must now be REPORTED in the finish message, not merely run. That run skipped it, leaving 16 cache entries and no hit rate anyone could see. 285 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.