Conversation
Parameterize ask_holmes evals across an on/off tool_suggestions matrix (mirroring the model and env_config matrices). When the variant is "on", the SUGGEST_RUNBOOKS frontend noop tool is injected into the per-test ToolExecutor and a matching system-prompt block is appended so the LLM has the option to silently emit "skill" memories at the end of an investigation. After each run the suggestions emitted by the LLM are captured from result.tool_calls and surfaced in three places so regression CI reports can compare with/without and developers can drill into what memories would have been generated: - pytest user_properties (tool_suggestions, memories_count, suggested_memories) → consumed by the GitHub markdown report - New "Suggest" and "Memories" columns in the GitHub eval table - Braintrust span metadata + tags so the suggestions are visible in individual eval traces Default matrix runs both variants; TOOL_SUGGESTIONS_CONFIGS=on|off restricts the run to a single variant for local debugging. Signed-off-by: Claude <noreply@anthropic.com>
Ruff F541: the SUGGEST_RUNBOOKS_TOOL_DESCRIPTION fragments and the trailing fragments of SUGGEST_RUNBOOKS_SYSTEM_PROMPT had no interpolation placeholders, so the f-prefix was invalid and would fail linting in CI. Signed-off-by: Claude <noreply@anthropic.com>
The first run on PR #1991 captured a single memory titled "NetworkPolicy label mismatch causing timeouts between services" — that's a transient root-cause conclusion (once fixed, useless), not the durable thing we want. The Hermes Agent / RunbookHermes skill model is procedural memory: when-to-use + procedure + pitfalls. The offending bullet was in the tool description itself: - "Root cause patterns (what symptoms map to what causes)" which biases the model toward conclusions from THIS incident rather than the investigation methodology for the next one. Rework the tool description and parameter docstrings so the model captures access / query patterns: - title: framed as "Investigating X" or "<env-context>: how to diagnose Y", with explicit GOOD/BAD examples - symptoms: when-to-use trigger, recognizable patterns rather than "what was wrong this time" - instructions: ordered procedure ("what to query FIRST / LAST / SKIP", environment-specific access patterns, tool quirks) - explicit DO NOT list: root-cause conclusions, specific resource names/timestamps, generic Kubernetes advice Verified with opus-4.6 (the production model we care about): the revised prompt now produces titles like "Investigating missing environment variables causing CrashLoopBackOff" with FIRST/SECOND investigation steps, instead of conclusion-style summaries. Signed-off-by: Claude <noreply@anthropic.com>
Previous framings were still too broad: capturing "investigation
methodology" or "what to check first" feeds the LLM things it already
knows from training — that's noise, not durable knowledge.
The actual goal is much narrower: capture ONLY when, this turn, the
LLM made a tool call with the wrong parameters (empty/error/wrong
data) and then succeeded with different parameters that required
environment-specific knowledge a fresh LLM would not have guessed.
That single correction — the working call shape for THIS environment
— is the only thing worth saving so the next investigation skips the
failed attempt.
Rewrite tool description and schema accordingly:
- Description: explicit DO/DON'T list, lead with "ONLY call this
tool if your investigation contained a wrong-call→right-call pair
that required env-specific knowledge". Generic mistakes any LLM
would self-correct (typos, missing -n, --previous on a crashed
container) are explicitly excluded.
- Schema: replace symptoms/instructions/alerts with the four fields
that capture the correction itself —
* failed_call: shape of the call that didn't work
* working_call: shape of the call that does work in this env
* when_to_use: how to recognize the skill is relevant next time
* why_env_specific: one-sentence reason a fresh LLM wouldn't
have guessed the right call (self-check — if you can't write
it, the correction is generic and should be skipped)
Update the unit test that exercises the schema to use a real-looking
correction (PromQL label override) instead of the old symptoms/
instructions/alerts shape.
Signed-off-by: Claude <noreply@anthropic.com>
Schema:
- AskHolmesTestCase gains optional `memories_generated: bool | None`.
- True → suggest=on variant fails if zero memories were emitted.
- False → suggest=on variant fails if any memory was emitted.
- None → no count enforcement (existing behavior).
- Emitted memories are surfaced into the LLM judge's evaluation_output on
the suggest=on variant so memory content quality is scored against the
eval's expected_output exactly like the final answer. The judge prompt
is instructed to fail the test when a required memory is missing or
captures the wrong thing.
All 11 existing regression evals get `memories_generated: false` as a
safety net so any spurious memory generation in CI fails loudly.
Two new regression evals exercise real failed→succeeded tool-call
corrections that require env-specific knowledge:
- 259_custom_label_selector — a payment-api deployment named pmt-svc-v2
with only the team's custom `acme.io/service` label (no `app=` label).
Asking by name forces the LLM to try `-l app=payment-api` first, hit
empty, discover the label via `--show-labels`, and re-query with the
custom label.
- 260_custom_namespace_naming — the cluster uses
`team-<team>-<service>-<env>-<id>` namespaces, not the default
`<service>`/`<service>-<env>`. The LLM has to enumerate namespaces
after the obvious guesses fail.
Both new evals have `memories_generated: true`, so suggest=on must
emit at least one memory and the judge scores its content via the
expected_output element ("There are N ready X replicas / running...").
Signed-off-by: Claude <noreply@anthropic.com>
A test that fails on an assertion AFTER update_test_results (e.g. the max_tokens limit or the new memories_generated check) was showing :white_check_mark: in the GitHub markdown report because TestStatus.passed only looked at actual_correctness_score — which was set to 1 by the judge before the assertion fired. - TestStatus.passed now also requires pytest's status to be "passed" (or empty for legacy paths), so any assertion that fires after the judge correctly surfaces as red in the report. - The memories_generated check also resets actual_correctness_score to 0 in user_properties before asserting, so the score column in the report matches the failed status. - Unit test in tests/test_test_results.py guards the regression. Signed-off-by: Claude <noreply@anthropic.com>
CodeRabbit nitpick: CLAUDE.md requires imports at the top of the file, not inside functions. The local `import json as _json` inside update_test_results moves to module scope and the alias is dropped. Signed-off-by: Claude <noreply@anthropic.com>
The 259/260 evals were too weak: an efficient LLM can sidestep the
"wrong" call entirely (e.g. by listing labels or namespaces up front
rather than guessing the standard ones first). When that happens
there is no real failed→succeeded correction to capture, and our
`memories_generated: true` assertion produces false-positive failures.
Replace them with three evals where the wrong call is near-inevitable
because the "right" call requires environment-specific knowledge a
fresh LLM has no way to guess:
- 261_es_log_severity_field — index has only a `severity` field, no
`level`. `level:"ERROR"` returns zero hits; the agent must read
the mapping and re-query with `severity:"ERROR"`.
- 262_es_index_via_alias — production logs are only reachable via
the alias `app-262-prodlogs` (real index is a date-stamped
rotation). Obvious raw names 404; the agent must `GET _alias`.
- 263_loki_custom_stream_label — Promtail labels streams with
`acme_service` instead of `service`/`app`. `{service="checkout"}`
matches no streams; the agent must list label names first.
All three are tagged `regression` and have `memories_generated:
true`. Each demonstrates a tool-call correction where the lesson is
genuinely durable (applies to future investigations in the same
environment) and would not be in any LLM's training data.
Signed-off-by: Claude <noreply@anthropic.com>
Without a toolsets.yaml in the fixture folder the elasticsearch/data and elasticsearch/cluster toolsets stay disabled, so the agent answers "Elasticsearch is not configured" instead of investigating — which masks whether the eval actually exercises the wrong-then-right correction it's designed to. Mirrors the config from 183b_elasticsearch_index_discovery. Signed-off-by: Claude <noreply@anthropic.com>
System-prompt rewrite — the addition we inject when suggest_runbooks
is enabled now explicitly explains the goal (skip the failed call
next time in THIS environment) and names the concrete patterns we
want captured: non-standard labels/selectors, non-standard metric
names (e.g. team prefix replacing the upstream kafka_* / mysql_*
family), non-standard log field shapes, non-standard data
locations / addressing, custom CRDs, and tool-routing quirks.
Each capture should record the failed_call vs working_call shape —
the env-specific things the LLM did not know before this run.
Reiterates the don't-capture list (generic methodology already in
the model's training data, transient root causes) and keeps the
"never acknowledge calling this tool" directive.
Evals:
- Drop 262_es_index_via_alias — structurally weak; the agent uses
elasticsearch_list_indices reflexively and the response leaks
the real index name, so no wrong-then-right pair ever happens.
- Fix 261_es_log_severity_field timestamp bug — "0${i}" produced
invalid ISO for i=10..15, so ES silently rejected 7 docs and the
expected count couldn't be met.
- Add 262_prometheus_custom_kafka_metric_name — exporter publishes
Kafka metrics under acme_kafka_* instead of the upstream kafka_*
family. A fresh LLM defaulting to kafka_server_brokertopicmetrics_*
or kafka_topic_* gets no series and has to enumerate metric names
to find the team prefix. The lesson is genuinely env-specific:
"Kafka metrics in this cluster live under acme_kafka_*" — not
knowledge any LLM has from training.
Signed-off-by: Claude <noreply@anthropic.com>
opus-4.6 was skipping the wrong-then-right pattern by checking elasticsearch_mappings first (smart!) — so the agent never tried the conventional level:"ERROR" query and there was no env-specific correction to capture. Re-phrase the user prompt to ask explicitly about "level=ERROR" so the agent's first natural query uses the standard log-field name, returns zero hits, and then has to discover via the mapping that this index uses `severity`. The durable lesson stays the same: "this index uses severity, not level — query severity:\"ERROR\" after confirming via mapping." Signed-off-by: Claude <noreply@anthropic.com>
Even with the biased 261 prompt forcing opus-4.6 into a textbook
wrong-then-right pattern (level:"ERROR" → empty → mapping discovery
→ severity:"ERROR" → success), and even when the agent SPELLS OUT
the lesson in its own answer ("this index uses severity, not
level"), it still doesn't emit a suggest_runbooks call. The
examples in the prompt weren't enough.
Add an explicit four-step checklist the agent must run BEFORE
finalizing its answer: (1) scan tool history, (2) did any call
return empty followed by a successful call with different params,
(3) was the difference env-specific, (4) if yes, emit the
suggest_runbooks call in the SAME response as the final answer.
Also makes explicit that mentioning the correction in prose is NOT
a substitute for emitting the tool call — the prose is read by the
current user; the tool call surfaces a save-able chip for future
investigations.
Signed-off-by: Claude <noreply@anthropic.com>
With the workflow checklist added, opus-4.6 started emitting suggest_runbooks — but as a SEPARATE turn AFTER its final answer. That consumes the turn that would otherwise contain the user-facing answer text, leaving result.result empty: the user sees no answer and the eval judge would correctly fail correctness. Anthropic / OpenAI models support multiple tool_use blocks in one assistant message (parallel tool calls). Update the checklist to spell that out explicitly: emit the suggest_runbooks call ALONGSIDE the answer prose in the SAME assistant message. Also tighten the noop response so that if the model still ends up there as a separate step it knows to produce the answer text NOW. Signed-off-by: Claude <noreply@anthropic.com>
Parallel-with-answer didn't reliably trigger; opus-4.6 chose one or the other (memory ✓ + empty answer, or full answer + no memory). Switch to a clean three-step ordering: (1) detect correction condition; (2) emit suggest_runbooks first — tool returns silently; (3) write the final answer text in the next message. This keeps the agentic loop happy: tool call → noop response that explicitly says "user has not seen your answer yet, write it now" → assistant emits answer text. The noop response also reframed to keep the model from interpreting the silent return as a stop signal: it must produce answer text in the very next message. Signed-off-by: Claude <noreply@anthropic.com>
Adds an optional `rerun_with_memory: true` field to AskHolmesTestCase. When set, after the first eval pass captures a memory the harness runs the same prompt a SECOND time with each emitted memory rendered as a SKILL.md file under a tempdir, and the SkillsToolset's search paths extended to include that tempdir. The replay flow: 1. Memories from pass-1 are serialized to SKILL.md files with name, description, and a structured body (when to use / failed call shape / working call shape / why env-specific). 2. TestToolsetManager now accepts `additional_skill_paths` so the standard skills toolset can scan the tempdir. 3. The ask_holmes helper now accepts the same kwarg and threads it through; the replay invokes ask_holmes again with suggest=off (no memory regeneration expected on the second pass). 4. The replay enforces two things: (a) the LLM must have called fetch_skill (proving it recognized the memory as relevant), and (b) the answer must still be correct. Both feed hard assertions. 5. New user_properties carry replay metrics — replay_attempted, replay_skill_loaded, replay_correctness, replay_turns, replay_tool_calls_count — picked up by the conftest collector. 6. The GitHub markdown reporter emits a second `[replay]` row right under each row that triggered a replay, with the skill-loaded status badge in the Memories column and the replay turns / tool call counts. The status icon mirrors the replay correctness so regressions are visible side-by-side with the first pass. Enabled on 261, 262, 263 so the rerun_with_memory loop is part of the regression run from the next push onward. Signed-off-by: Claude <noreply@anthropic.com>
Two problems found in the previous run for 0f0d8ec: 1. The Prometheus readiness check used an unencoded URL with curly braces and quotes inside the query string (`acme_kafka_msgs_consumed_total{topic="payments"}`). Many wget builds reject that as an invalid URL. Switch to the simple `up` metric scoped to our job — no special chars. On failure dump the targets endpoint and exporter logs so future regressions are diagnosable. 2. The exporter was nginx + a ConfigMap-mounted text file with an `alias` directive pointing at the mounted symlink. ConfigMap mounts surface each key via a chain of symlinks (`<dir>/<name>` -> `..data/<name>` -> a real file under a timestamped `..NNN/` directory) which nginx alias did not serve reliably in this combo. Replace it with a tiny Python http.server that bakes the metric text into a constant — far fewer moving pieces, a built-in readinessProbe on /metrics, and the same observed externally. Signed-off-by: Claude <noreply@anthropic.com>
The [replay] row was only filling in turns / tools / skill-loaded status and rendering em dashes for everything else. That obscured the whole point of the comparison — eyeballing whether the captured memory actually saves time, tool calls, and tokens. Capture the full LLMResult on the replay (duration, total_cost, total_tokens, prompt/completion/cached/reasoning, max-per-call, num_compactions) into user_properties, plumb them through the conftest collector, and render them in the replay row using the same formatting helpers as the original row. Side-by-side rows now make the win obvious: e.g. main 22.4s / 7 turns / 10 tools / 120K tokens vs replay 13.2s / 5 turns / 6 tools / 85K tokens. Signed-off-by: Claude <noreply@anthropic.com>
The replay row's status icon used only replay_correctness, ignoring replay_skill_loaded. When the assert fetch_skill_called fires after the correctness score is logged, replay_correctness stays at 1 in user_properties — the report showed ✅ even though pytest had failed the test for the missing skill load. Require BOTH replay_correctness == 1 AND replay_skill_loaded for the green checkmark. Either one false yields ❌, matching what pytest actually reported. The Memories column already showed "skill ✗" so the inconsistency between badge and icon disappears. Signed-off-by: Claude <noreply@anthropic.com>
The previous report still lacks duration / cost / tokens on the replay row because that eval run pre-dates 91aea68 (which capture full LLMResult stats for the replay). Bumping the branch so CI re-runs against 388d300 — that should produce a replay row with full stats and the corrected red-on-missing-skill status icon. Signed-off-by: Claude <noreply@anthropic.com>
…av1Uv Resolved three conflicts in tests/llm/utils/reporting/github_reporter.py where master added a Src column at the end of each table row and my branch added Suggest/Memories columns near the front plus the [replay] sub-row. The merged version keeps both: 18-column header (Status, Test case, Suggest, Memories, Time, Turns, Tools, Cost, Total tokens, Input, Max input, Output, Max output, Cached, Non-cached, Reasoning, Compactions, Src), per-row markdown emits the matrix Suggest/Memories cells AND the Src link, the [replay] sub-row reuses the parent row's Src link (same test_case.yaml), and the Totals row keeps the matrix aggregates plus the empty Src cell. Signed-off-by: Claude <noreply@anthropic.com>
When the closed-loop replay assertion fires, pytest marks the whole test as failed. The github report would then paint BOTH the primary row and the [replay] row red, even though the primary pass (memory emission, judge correctness) actually succeeded — only the replay loaded the wrong / no skill. Track primary_passed in user_properties immediately after the primary assertions pass (before the replay block runs). The report uses primary_passed (when a replay was attempted) to render the primary row's icon, so 263-style situations now show: ✅ 263_loki ... on 1 memory ok, primary correct ❌ 263_loki [replay] ... skill ✗ replay failed independently Two independent measurements, two independent icons. Adds .goal_criteria.md documenting the up-front criteria for the sub-agent improvement goal (30% token reduction on ≥5 evals without regressing the existing 11 regression evals by >10%). Signed-off-by: Claude <noreply@anthropic.com>
…_use 264 covers the case where the agent has to discover that log level is encoded as a numeric severity_num scale (1-6) rather than a text 'level' field. Validated locally: replay drops from 184,739 → 106,164 tokens (-42.5%) when the captured memory is rendered as a fetched skill. The skill description shown to the agent now leads with the captured when_to_use phrase (the symptom/query shape), then appends the title (the durable env-specific lesson). The previous title-only description matched on the lesson keyword but not on what the user actually asks about, so fetch_skill was sometimes not triggered on replay. https://claude.ai/code/session_01NkgEXcoZhJD498VPmsYXmb Signed-off-by: Claude <noreply@anthropic.com>
263: The previous prompt let the agent reach loki_list_labels as its
natural first step, so there was no wrong→right correction to capture
and the suggest=on variant never emitted a memory. New prompt names the
candidate selector `{service="checkout"}` explicitly so the agent
commits to it first, gets an empty stream, then has to list labels and
retry with `{acme_service="checkout"}`. That correction is the durable
env-specific lesson the eval was designed to teach.
265: New eval — index has NO `@timestamp` date field at all; the only
time column is a custom `ingest_ts` typed as keyword. The prompt names
`@timestamp` explicitly so the agent's first range query is against a
non-existent field; it then has to inspect the mapping and rewrite the
filter as a string comparison on `ingest_ts`. Same wrong→right pattern
as 261/264.
https://claude.ai/code/session_01NkgEXcoZhJD498VPmsYXmb
Signed-off-by: Claude <noreply@anthropic.com>
The previous rigid 'Use the @timestamp field for the time filter' caused the agent to obey literally and answer 0 instead of discovering the renamed `ingest_ts` field and answering with 7. Removing the field-name instruction lets the agent reach for `@timestamp` naturally (as the ES default), fail, inspect the mapping, find `ingest_ts`, and return the right count. Memory + replay fetch_skill confirmed locally. https://claude.ai/code/session_01NkgEXcoZhJD498VPmsYXmb Signed-off-by: Claude <noreply@anthropic.com>
The previous prompts ("how many entries have level=ERROR?") were too
implicit: in some CI runs the agent peeked at the index mapping first
and went straight to the working field, so there was no wrong-then-right
correction to capture and memories_generated=true failed. The 263 prompt
(which already names the wrong selector explicitly and tells the agent
to find the right one if it returns nothing) reliably elicits the
correction; copy that pattern for 261, 264, 265.
- 261: ``term: { level: "ERROR" }`` named explicitly
- 264: ``terms: { level: ["ERROR", "FATAL"] }`` named explicitly
- 265: ``range: { "@timestamp": {gte: ...} }`` named explicitly
In all three cases the prompt now ends with "if that returns zero, find
the right field/timestamp and re-run" so the agent still produces the
correct answer after the wrong call.
https://claude.ai/code/session_01NkgEXcoZhJD498VPmsYXmb
Signed-off-by: Claude <noreply@anthropic.com>
…eric The previous "do NOT call for" list included "look at mapping when query is empty" as a generic methodology example. That phrasing read as "any discovery via mapping inspection is generic" and made the agent classify env-specific field-name corrections (severity vs level, acme_service vs service, ingest_ts vs @timestamp) as not memory-worthy because the *method* used to find them (inspect mapping / list labels) was on the don't-capture list. Reword: the *method* of inspecting/listing IS generic, but the FACT you discover (the specific field name this team uses) is env-specific and IS worth capturing. Verified locally: 261 now reliably emits memory + replay drops 156k→108k (-31.0%) when fetched as a skill. https://claude.ai/code/session_01NkgEXcoZhJD498VPmsYXmb Signed-off-by: Claude <noreply@anthropic.com>
Previously the SKILL.md body went: When to use → Failed call → Working call → Why env-specific. The agent on replay would read the skill and then still run discovery (label list, mapping inspection) to verify, limiting replay token savings (263 saw only -20% reduction because the discovery wasn't skipped). New layout: When to use → What to do (skip discovery, use working call directly) → Working call (front-and-center) → Failed call → Why env-specific. The "What to do" section explicitly tells the agent to bypass the discovery step the skill exists to replace. https://claude.ai/code/session_01NkgEXcoZhJD498VPmsYXmb Signed-off-by: Claude <noreply@anthropic.com>
The previous expected_output was "8 entries with ERROR or FATAL severity (severity_num >= 5)" — the parenthetical jargon doesn't match how the agent typically writes the answer (it explains the mapping in prose rather than as a range expression), so the LLM judge marked it wrong even when the count was correct. New expected mirrors 261's style: states the count + severity + index, then explains the env-specific quirk (numeric encoding) in plain language that matches how the agent writes its answer. https://claude.ai/code/session_01NkgEXcoZhJD498VPmsYXmb Signed-off-by: Claude <noreply@anthropic.com>
The CI run on 888dc16 showed the directive section regressed three evals: 261/263 dropped from skill ✓ to skill ✗ (the agent stopped fetching the skill at all), and 265 token usage went UP +24.9% because the additional directive text in the skill body cost more than it saved. The simpler body (When-to-use → Failed → Working → Why) worked better — bf5797a saw 261 ✓ at -29.5%, 263 ✓ skill-loaded, 265 ✓ skill-loaded. Reverting the directive while keeping the working changes: - biased prompts (360b1f2) - system prompt fix ("look at mapping" is method, field-name is fact) - 264 expected_output clarification (0848dbd) https://claude.ai/code/session_01NkgEXcoZhJD498VPmsYXmb Signed-off-by: Claude <noreply@anthropic.com>
261/262/263/264/265 are the evals designed for the SUGGEST_RUNBOOKS + rerun_with_memory closed loop. Add the existing 'skills' tag so we can run just this baseline with pytest -m skills. https://claude.ai/code/session_01NkgEXcoZhJD498VPmsYXmb Signed-off-by: Claude <noreply@anthropic.com>
The on/off matrix doubled every eval run for a comparison we no longer need: the SUGGEST_RUNBOOKS tool is now standard equipment for ask_holmes evals, and the off variant was only useful while we were validating that the feature didn't regress non-skills tests. With that validation done, keeping the matrix wastes ~50% of every CI run on a column that always reads the same. This collapses the dimension entirely: - tool_suggestions_config.py: drop ToolSuggestionsConfig, the env-var parser, the matrix getter, and the Tuple[ai, bool] injection shim. inject_suggest_runbooks_tool(ai) now always injects; append_suggest_runbooks_system_prompt(prompt) now always appends. - test_ask_holmes.py: remove the @pytest.mark.parametrize on tool_suggestions and the `[suggest=on/off]` trace-name suffix. The primary pass always injects; the closed-loop replay passes inject_suggest_runbooks=False. - Report rendering: drop the Suggest column and the tool_suggestions_enabled gating around the Memories column. Memories is always a numeric count now; the totals row drops "N/M on" in favor of the raw memories sum. - Tracking: property_manager, conftest, and braintrust stop emitting tool_suggestions / tool_suggestions_enabled fields; the replay-only memories signal is unchanged. - Fixtures: rewrite the "On suggest=on ..." narrative comments on the 261-265 skills evals — they referenced a matrix variant that no longer exists. - Unit tests: drop the matrix-parsing tests, keep the extract_suggested_memories coverage. 256 tests collected after this change (was ~512) and the test_tool_suggestions_config.py suite passes. Signed-off-by: Claude <noreply@anthropic.com>
Three changes aimed at making the skills mechanism's net win visible
and stress-testing it against realistic conditions:
1. **Skills net-win summary in the GitHub report.** Right after the
totals row, the report now prints a small block aggregating:
how many evals emitted a memory, how many replays were attempted,
how often the skill loaded, how often the answer was still correct,
and the per-row average primary→replay cost/token delta. Lets the
"is this feature paying for itself" answer be cited in one sentence
without eyeballing every row.
2. **Soften prompts on 261 and 263.** Both fixtures previously
prescribed the failure path explicitly ("Run X. If returns zero,
find Y."). That tested obedience, not discovery. The new phrasings
ask realistic, unbiased questions and trust the agent to discover
the env-specific quirk on its own. If the agent still emits the
captured memory after recovering, we've shown the mechanism works
on realistic queries; if it doesn't, we've found a prompt
sensitivity worth fixing.
3. **Bad-memory regression eval (267_bad_skill_resilience).** Adds a
`pre_loaded_skills_path` field to `AskHolmesTestCase` — when set,
the fixture's named directory is added to the SkillsToolset's
search paths BEFORE the primary pass. Fixture 267 uses this to
pre-load a deliberately misleading SKILL.md that claims the ES
index uses `lvl: "ERR"` when it actually uses `severity: "ERROR"`.
The test asserts the agent still arrives at the correct count
(10 ERROR rows) despite the bad guidance, proving captured skills
can't permanently mislead.
Out of scope here (per discussion): cross-transfer eval (concern 2),
production wiring of memory persistence (concern 5), and dedicated
accumulated-memories noise testing (concern 4).
Signed-off-by: Claude <noreply@anthropic.com>
Two fixes driven by Braintrust trace inspection of the prior run: **267_bad_skill_resilience didn't actually test bad-skill resilience.** The agent fetched the misleading skill correctly, but couldn't run the ES query because the elasticsearch/data toolset was never enabled for the fixture — the agent ended with "The elasticsearch/data toolset is disabled. I cannot execute the search query directly." Added a toolsets.yaml mirroring 261's so the agent can actually run the wrong query and discover the correction. Next run will be the first real test of whether the agent recovers from a misleading captured skill. **Reverted softened prompts on 261 and 263.** The trace for 261 was revealing: with the un-biased "how many ERROR entries are there" phrasing, the agent answered correctly on the first try (`severity == "ERROR"`) — never made the wrong call, so suggest_runbooks correctly emitted zero memories. The test's `memories_generated: true` assertion then failed. That isn't a regression — it's the correct behavior. The mechanism is designed for wrong→right corrections, and when the agent's first call is right, there's nothing to teach. We learned that softer prompts elide the failure-then-recovery shape the mechanism depends on. The biased phrasing reliably triggers the correction (and reliable emission was the point of this fixture), so restore it. The fixture comment now documents what we learned, including why a separate "natural-phrasing emission rate" eval would be a different test. Signed-off-by: Claude <noreply@anthropic.com>
Trace analysis of the prior run surfaced the real reason 261/263
replays kept failing with `skill ✗`: the prompts that work for primary
emission *block* skill use on replay.
The biased phrasing on 261/263 ("Run X. If returns zero, find Y.")
prescribes the recovery path. On the primary pass that's good — the
agent runs the wrong call, gets nothing, recovers, and emits a memory
captured from the correction. On replay the same prompt is still telling
the agent the recovery path, so it confidently does the wrong→right
dance again and never bothers to fetch the captured skill — the whole
point of which is to skip the failed call.
262 was succeeding on replay because its primary prompt is already a
natural, un-biased question ("how many messages on the payments
topic?") — the agent doesn't know the metric name, so it checks
skills and finds the one. 261/263 needed the same affordance for the
replay step only.
Fix: add `replay_user_prompt` on AskHolmesTestCase. When set, the
closed-loop replay run uses it instead of the original `user_prompt`.
This models the real-world scenario the mechanism is designed for:
the first investigation has rich detail about what went wrong (the
biased eval prompt analog); the second investigation asks the same
question naturally and the captured skill should pay off by
short-circuiting the failed call.
Wiring:
- `AskHolmesTestCase.replay_user_prompt` (Optional[str|List[str]])
- `ask_holmes(..., override_user_prompt=...)` substitutes when present
- Replay block in test_ask_holmes passes
`getattr(test_case, "replay_user_prompt", None)` through
Applied to 261, 263, 265 (the three biased-prompt skills evals).
Expected delta next run: replays that previously hit skill ✗ should
load the skill and short-circuit the failed call.
Signed-off-by: Claude <noreply@anthropic.com>
Data from the prior CI run was mixed: replay_user_prompt fixed 261 (skill ✗ → skill ✓, -25% cost / -39% tokens) but broke 265 (skill ✓ correct → skill ✓ but wrong answer, +20% cost / +39% tokens). The soft "from today" phrasing on 265 caused the agent to load the captured skill yet still compute the date range incorrectly — the explicit "@timestamp gte 2026-06-03" in the primary prompt was load- bearing for the right answer. Keeping the mechanism and the working applications (261, 263) and reverting just 265 to use the original biased prompt for replay. Note in the fixture documents why. 263's replay still misses fetch_skill regardless of phrasing — the agent recovers via mapping/label inspection without consulting the skill catalog. That's a structural limit for this eval, not a prompt-phrasing issue, and not something replay_user_prompt can fix. Signed-off-by: Claude <noreply@anthropic.com>
…ce, not per quirk) The per-quirk skill design from claude/add-tool-suggestions-matrix-av1Uv has a known scaling problem: a real customer's elasticsearch has dozens of schema quirks across dozens of indices, and emitting one SKILL.md per quirk produces a catalog so large the SkillsToolset listing in the system prompt becomes a burden of its own. It also means the agent has to fetch and merge multiple skills per investigation. This branch shifts the contract: every emission must declare a `skill_domain` (`elasticsearch`, `loki`, `prometheus`, `kubernetes`, `grafana`, ...). The harness groups emissions by domain and writes ONE "Known quirks for querying <domain>" SKILL.md per domain, with each quirk rendered as a numbered entry in a `## Known quirks` body. A future investigation that uses that data source fetches one skill and sees every quirk this team's environment has. Mechanism changes: - suggest_runbooks tool schema: add required `skill_domain` field. - System prompt: explicit CONSOLIDATION section telling the agent to pick the coarsest stable data-source name and reuse it across all quirks for that source in one investigation. - write_memories_as_skill_files: groups by `skill_domain` via an OrderedDict, normalizes domain strings to lowercase-hyphenated slugs, writes one SKILL.md per group with a multi-quirk body. Falls back to domain="general" if absent so older emissions don't crash. - AskHolmesTestCase: new `expected_skill_count` field. Asserts the number of written files equals the declared value — catches the failure mode where the agent invents N different domains instead of consolidating. Eval changes: - New fixture 268_es_multi_quirk_consolidation. Sets up three ES indices each with one quirk (severity/severity_num/ingest_ts — borrowed from 261/264/265 patterns), asks one question per index in one prompt. Expects three quirks captured all tagged `skill_domain: "elasticsearch"`, consolidating into one file (expected_skill_count: 1). Replay asks the same three questions in a natural phrasing and verifies the agent fetches that one consolidated skill and answers all three. - Unit tests cover the consolidation logic directly: 3 memories across 2 domains → 2 files (with the ES file containing both ES quirks), and the fallback-domain case for legacy un-tagged emissions. Existing 261-265 fixtures keep working: their single-quirk emissions become single-entry domain skills. The new mechanism only adds shape; it doesn't reject the old shape. Out of scope (next iteration if this approach lands): - Real update-existing semantics (in production a domain skill would be a DB upsert across investigations; here the harness merges within a single eval run only). - Cross-domain skills (e.g. correlating Kubernetes events with Loki logs). Branches off claude/add-tool-suggestions-matrix-av1Uv @ 4d27a91, preserving that branch as a shippable fallback if this approach doesn't pan out. Signed-off-by: Claude <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
📂 Previous Runs
|
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Max input | Output | Max output | Cached | Non-cached | Reasoning | Skill Generated | Skills Read | Compactions | Denied commands | Src |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ✅ | 09_crashpod | 33.8s | 4 | 8 | $0.2428 | 83,651 | 81,872 | 22,912 | 1,779 | 685 | 58,159 | 23,713 | 218 | — | — | — | — | src |
| ✅ | 101_loki_historical_logs_pod_deleted | 47.1s | 5 | 10 | $0.2903 | 108,467 | 105,883 | 25,222 | 2,584 | 789 | 80,372 | 25,511 | 239 | — | — | — | — | src |
| ✅ | 112_find_pvcs_by_uuid | 15.3s | 2 | 2 | $0.1801 | 39,956 | 39,088 | 21,353 | 868 | 437 | 17,732 | 21,356 | 269 | — | — | — | — | src |
| ✅ | 12_job_crashing | 31.9s | 5 | 11 | $0.2613 | 109,372 | 107,641 | 24,091 | 1,731 | 441 | 83,126 | 24,515 | 66 | — | — | — | — | src |
| ✅ | 176_network_policy_blocking_traffic_no_skills | 43.9s | 5 | 11 | $0.2710 | 110,300 | 108,329 | 24,853 | 1,971 | 615 | 83,470 | 24,859 | 349 | — | — | — | — | src |
| ✅ | 227_count_configmaps_per_namespace[0] | 18.6s | 3 | 6 | $0.1774 | 56,609 | 55,893 | 20,075 | 716 | 432 | 35,814 | 20,079 | 37 | — | — | — | — | src |
| ✅ | 243_pod_names_contain_service | 25.8s | 3 | 7 | $0.2082 | 59,661 | 58,280 | 21,243 | 1,381 | 660 | 36,266 | 22,014 | 162 | — | — | — | — | src |
| ✅ | 24_misconfigured_pvc | 36.2s | 6 | 11 | $0.2639 | 126,658 | 124,949 | 22,668 | 1,709 | 574 | 101,306 | 23,643 | 28 | — | — | — | — | src |
| ✅ | 254_elasticsearch_dr_test_log_check | 71.0s | 10 | 13 | $0.3524 | 175,106 | 171,188 | 22,615 | 3,918 | 915 | 147,071 | 24,117 | 241 | 1 | — | — | — | src |
| ✅ | 259_wrong_cluster_logs_confusion | 88.2s | 11 | 14 | $0.3894 | 196,125 | 191,423 | 23,018 | 4,702 | 973 | 166,536 | 24,887 | 748 | 1 | — | — | — | src |
| ✅ | 260_global_es_remote_cluster_logs | 70.4s | 10 | 12 | $0.3307 | 175,730 | 172,253 | 21,509 | 3,477 | 718 | 149,878 | 22,375 | 227 | 1 | — | — | — | src |
| ✅ | 261_es_log_severity_field | 20.8s | 4 | 3 | $0.2653 | 113,740 | 111,930 | 24,215 | 1,810 | 558 | 87,545 | 24,385 | 282 | 1 | — | — | — | src |
| ✅ | 261_es_log_severity_field [replay] | 21.6s | 4 | 3 | $0.1944 | 77,720 | 76,843 | 20,238 | 877 | 325 | 56,600 | 20,243 | 171 | — | — | — | — | src |
| ✅ | 262_prometheus_custom_kafka_metric_name | 14.0s | 3 | 2 | $0.2352 | 94,855 | 93,839 | 24,598 | 1,016 | 407 | 69,236 | 24,603 | 109 | 1 | — | — | — | src |
| ✅ | 262_prometheus_custom_kafka_metric_name [replay] | 14.5s | 3 | 2 | $0.1806 | 62,270 | 61,848 | 21,342 | 422 | 177 | 40,502 | 21,346 | 83 | — | — | — | — | src |
| ✅ | 263_loki_custom_stream_label | 29.3s | 5 | 4 | $0.2199 | 98,392 | 97,300 | 21,552 | 1,092 | 477 | 75,742 | 21,558 | 113 | 1 | — | — | — | src |
| ✅ | 263_loki_custom_stream_label [replay] | 29.8s | 5 | 4 | $0.1914 | 82,932 | 81,704 | 17,806 | 1,228 | 414 | 63,892 | 17,812 | 212 | — | — | — | — | src |
| ✅ | 264_es_numeric_severity_field | 39.1s | 5 | 7 | $0.3091 | 141,728 | 139,262 | 25,906 | 2,466 | 630 | 113,189 | 26,073 | 438 | 1 | — | — | — | src |
| ✅ | 264_es_numeric_severity_field [replay] | 40.0s | 5 | 7 | $0.2614 | 104,246 | 101,993 | 22,716 | 2,253 | 838 | 79,271 | 22,722 | 470 | — | — | — | — | src |
| ✅ | 265_es_timestamp_keyword_field | 25.8s | 5 | 4 | $0.2723 | 114,973 | 112,947 | 24,418 | 2,026 | 637 | 88,523 | 24,424 | 366 | 1 | — | — | — | src |
| ✅ | 265_es_timestamp_keyword_field [replay] | 26.7s | 5 | 4 | $0.2198 | 99,542 | 98,360 | 21,059 | 1,182 | 320 | 77,295 | 21,065 | 141 | — | — | — | — | src |
| ✅ | 266_es_schema_discovery | 16.0s | 4 | 3 | $0.2307 | 108,706 | 107,680 | 22,640 | 1,026 | 286 | 85,034 | 22,646 | 61 | 1 | — | — | — | src |
| ✅ | 266_es_schema_discovery [replay] | 16.9s | 4 | 3 | $0.1862 | 77,046 | 76,400 | 19,988 | 646 | 197 | 56,407 | 19,993 | 50 | — | — | — | — | src |
| ✅ | 267_bad_skill_resilience | 61.1s | 9 | 8 | $0.3575 | 217,890 | 215,304 | 26,556 | 2,586 | 588 | 188,738 | 26,566 | 458 | 1 | — | — | — | src |
| ✅ | 268_es_multi_quirk_consolidation | 47.9s | 5 | 10 | $0.4746 | 195,752 | 189,845 | 32,058 | 5,907 | 1,212 | 156,788 | 33,057 | 710 | 3 | — | — | — | src |
| ✅ | 268_es_multi_quirk_consolidation [replay] | 48.8s | 5 | 10 | $0.3046 | 110,218 | 107,185 | 24,969 | 3,033 | 936 | 81,348 | 25,837 | 541 | — | — | — | — | src |
| ✅ | 269_es_skill_update_cross_investigation | 32.4s | 5 | 7 | $0.4276 | 250,435 | 246,417 | 28,707 | 4,018 | 770 | 217,699 | 28,718 | 939 | 1 | — | — | — | src |
| ✅ | 269_es_skill_update_cross_investigation [replay] | 33.2s | 5 | 7 | $0.2513 | 104,308 | 102,463 | 22,796 | 1,845 | 518 | 79,661 | 22,802 | 120 | — | — | — | — | src |
| ✅ | 43_current_datetime_from_prompt | 3.7s | 1 | — | $0.1252 | 17,841 | 17,719 | 17,719 | 122 | 122 | 0 | 17,719 | 78 | — | — | — | — | src |
| ✅ | 51_logs_summarize_errors | 21.1s | 3 | 2 | $0.1831 | 57,221 | 56,451 | 20,695 | 770 | 390 | 35,752 | 20,699 | 34 | — | — | — | — | src |
| ✅ | 61_exact_match_counting | 10.6s | 2 | 1 | $0.1398 | 35,967 | 35,750 | 18,051 | 217 | 148 | 17,696 | 18,054 | 30 | — | — | — | — | src |
| Total | 35.0s avg | 5.0 avg | 7.1 avg | $6.2078 | 2,689,135 | 2,641,243 | 32,058 | 47,892 | 1,212 | 2,095,672 | 545,571 | 6,202 | 14 | — | — | — |
Skills mechanism stats
- Evals that emitted at least one memory: 12
- Replays attempted: 8
- Replays where the agent loaded the captured skill: 8/8
- Replays that answered correctly: 8/8
- Mean replay vs primary delta (per-row average): ↓24% cost, ↓32% tokens (n=8)
Benchmark Comparison Details
Master baseline: latest master-* experiment (post-merge regression eval)
Status: 11 test/model combinations loaded
- master-27089078550 (created: 2026-06-07)
Benchmark baseline: latest ci-benchmark experiment on master
Status: 34 test/model combinations loaded
- ci-benchmark-27257681111 (created: 2026-06-10)
Time comparison (seconds):
| Test case | This branch | master (2d ago) | Δ vs master | benchmark (2h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 33.8s | 26.2s | ↑29% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 47.1s | 47.6s | ±0% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 15.3s | 10.6s | ↑45% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 31.9s | 27.6s | ↑16% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 43.9s | 32.6s | ↑35% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 18.6s | 13.2s | ↑41% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 25.8s | 28.8s | ↓11% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 36.2s | 31.4s | ↑15% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 71.0s | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 88.2s | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 70.4s | — | — | — | — |
| 261_es_log_severity_field (opus-4.6) 📄 | 20.8s | — | — | — | — |
| 262_prometheus_custom_kafka_metric_name (opus-4.6) 📄 | 14.0s | — | — | — | — |
| 263_loki_custom_stream_label (opus-4.6) 📄 | 29.3s | — | — | — | — |
| 264_es_numeric_severity_field (opus-4.6) 📄 | 39.1s | — | — | — | — |
| 265_es_timestamp_keyword_field (opus-4.6) 📄 | 25.8s | — | — | — | — |
| 266_es_schema_discovery (opus-4.6) 📄 | 16.0s | — | — | — | — |
| 267_bad_skill_resilience (opus-4.6) 📄 | 61.1s | — | — | — | — |
| 268_es_multi_quirk_consolidation (opus-4.6) 📄 | 47.9s | — | — | — | — |
| 269_es_skill_update_cross_investigation (opus-4.6) 📄 | 32.4s | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 3.7s | 3.1s | ↑21% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 21.1s | 18.8s | ↑12% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 10.6s | 7.1s | ↑49% | — | — |
| Total (all, n=23) | 35.0s | 22.5s | — | — | — |
| Comparable (m=11, b=0) | 26.2s | 22.5s | ↑17% | — | — |
Cost comparison:
| Test case | This branch | master (2d ago) | Δ vs master | benchmark (2h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | $0.2428 | $0.2032 | ↑20% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | $0.2903 | $0.2642 | ±0% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | $0.1801 | $0.1368 | ↑32% | — | — |
| 12_job_crashing (opus-4.6) 📄 | $0.2613 | $0.2173 | ↑20% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | $0.2710 | $0.2524 | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | $0.1774 | $0.1464 | ↑21% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | $0.2082 | $0.1872 | ↑11% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | $0.2639 | $0.2240 | ↑18% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | $0.3524 | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | $0.3894 | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | $0.3307 | — | — | — | — |
| 261_es_log_severity_field (opus-4.6) 📄 | $0.2653 | — | — | — | — |
| 262_prometheus_custom_kafka_metric_name (opus-4.6) 📄 | $0.2352 | — | — | — | — |
| 263_loki_custom_stream_label (opus-4.6) 📄 | $0.2199 | — | — | — | — |
| 264_es_numeric_severity_field (opus-4.6) 📄 | $0.3091 | — | — | — | — |
| 265_es_timestamp_keyword_field (opus-4.6) 📄 | $0.2723 | — | — | — | — |
| 266_es_schema_discovery (opus-4.6) 📄 | $0.2307 | — | — | — | — |
| 267_bad_skill_resilience (opus-4.6) 📄 | $0.3575 | — | — | — | — |
| 268_es_multi_quirk_consolidation (opus-4.6) 📄 | $0.4746 | — | — | — | — |
| 269_es_skill_update_cross_investigation (opus-4.6) 📄 | $0.4276 | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | $0.1252 | $0.0110 | ↑1039% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | $0.1831 | $0.1488 | ↑23% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | $0.1398 | $0.1112 | ↑26% | — | — |
| Total (all, n=23) | $0.2699 | $0.1730 | — | — | — |
| Comparable (m=11, b=0) | $0.2130 | $0.1730 | ↑23% | — | — |
Total tokens comparison:
| Test case | This branch | master (2d ago) | Δ vs master | benchmark (2h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 83,651 | 67,778 | ↑23% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 108,467 | 88,307 | ↑23% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 39,956 | 30,686 | ↑30% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 109,372 | 69,487 | ↑57% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 110,300 | 108,726 | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 56,609 | 44,988 | ↑26% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 59,661 | 48,612 | ↑23% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 126,658 | 68,923 | ↑84% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 175,106 | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 196,125 | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 175,730 | — | — | — | — |
| 261_es_log_severity_field (opus-4.6) 📄 | 113,740 | — | — | — | — |
| 262_prometheus_custom_kafka_metric_name (opus-4.6) 📄 | 94,855 | — | — | — | — |
| 263_loki_custom_stream_label (opus-4.6) 📄 | 98,392 | — | — | — | — |
| 264_es_numeric_severity_field (opus-4.6) 📄 | 141,728 | — | — | — | — |
| 265_es_timestamp_keyword_field (opus-4.6) 📄 | 114,973 | — | — | — | — |
| 266_es_schema_discovery (opus-4.6) 📄 | 108,706 | — | — | — | — |
| 267_bad_skill_resilience (opus-4.6) 📄 | 217,890 | — | — | — | — |
| 268_es_multi_quirk_consolidation (opus-4.6) 📄 | 195,752 | — | — | — | — |
| 269_es_skill_update_cross_investigation (opus-4.6) 📄 | 250,435 | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 17,841 | 13,970 | ↑28% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 57,221 | 45,163 | ↑27% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 35,967 | 28,231 | ↑27% | — | — |
| Total (all, n=23) | 116,919 | 55,897 | — | — | — |
| Comparable (m=11, b=0) | 73,246 | 55,897 | ↑31% | — | — |
Cached tokens comparison:
| Test case | This branch | master (2d ago) | Δ vs master | benchmark (2h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 58,159 | 46,700 | ↑25% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 80,372 | 64,209 | ↑25% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 17,732 | 13,861 | ↑28% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 83,126 | 46,249 | ↑80% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 83,470 | 84,274 | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 35,814 | 28,072 | ↑28% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 36,266 | 28,690 | ↑26% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 101,306 | 46,110 | ↑120% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 147,071 | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 166,536 | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 149,878 | — | — | — | — |
| 261_es_log_severity_field (opus-4.6) 📄 | 87,545 | — | — | — | — |
| 262_prometheus_custom_kafka_metric_name (opus-4.6) 📄 | 69,236 | — | — | — | — |
| 263_loki_custom_stream_label (opus-4.6) 📄 | 75,742 | — | — | — | — |
| 264_es_numeric_severity_field (opus-4.6) 📄 | 113,189 | — | — | — | — |
| 265_es_timestamp_keyword_field (opus-4.6) 📄 | 88,523 | — | — | — | — |
| 266_es_schema_discovery (opus-4.6) 📄 | 85,034 | — | — | — | — |
| 267_bad_skill_resilience (opus-4.6) 📄 | 188,738 | — | — | — | — |
| 268_es_multi_quirk_consolidation (opus-4.6) 📄 | 156,788 | — | — | — | — |
| 269_es_skill_update_cross_investigation (opus-4.6) 📄 | 217,699 | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | 13,845 | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 35,752 | 28,012 | ↑28% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 17,696 | 13,825 | ↑28% | — | — |
| Total (all, n=23) | 91,116 | 37,622 | — | — | — |
| Comparable (m=10, b=0) | 54,969 | 40,000 | ↑37% | — | — |
Turns comparison:
| Test case | This branch | master (2d ago) | Δ vs master | benchmark (2h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 4 | 4 | ±0% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 5 | 5 | ±0% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 5 | 4 | ↑25% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 5 | 6 | ↓17% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 3 | 3 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 3 | 3 | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 6 | 4 | ↑50% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 10 | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 11 | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 10 | — | — | — | — |
| 261_es_log_severity_field (opus-4.6) 📄 | 4 | — | — | — | — |
| 262_prometheus_custom_kafka_metric_name (opus-4.6) 📄 | 3 | — | — | — | — |
| 263_loki_custom_stream_label (opus-4.6) 📄 | 5 | — | — | — | — |
| 264_es_numeric_severity_field (opus-4.6) 📄 | 5 | — | — | — | — |
| 265_es_timestamp_keyword_field (opus-4.6) 📄 | 5 | — | — | — | — |
| 266_es_schema_discovery (opus-4.6) 📄 | 4 | — | — | — | — |
| 267_bad_skill_resilience (opus-4.6) 📄 | 9 | — | — | — | — |
| 268_es_multi_quirk_consolidation (opus-4.6) 📄 | 5 | — | — | — | — |
| 269_es_skill_update_cross_investigation (opus-4.6) 📄 | 5 | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 1 | 1 | ±0% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 3 | 3 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| Total (all, n=23) | 5.0 | 3.4 | — | — | — |
| Comparable (m=11, b=0) | 3.5 | 3.4 | ±0% | — | — |
Tool calls comparison:
| Test case | This branch | master (2d ago) | Δ vs master | benchmark (2h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 8 | 8 | ±0% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 10 | 11 | ±0% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 11 | 9 | ↑22% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 11 | 12 | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 6 | 6 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 7 | 7 | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 11 | 13 | ↓15% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 13 | — | — | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 14 | — | — | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 12 | — | — | — | — |
| 261_es_log_severity_field (opus-4.6) 📄 | 3 | — | — | — | — |
| 262_prometheus_custom_kafka_metric_name (opus-4.6) 📄 | 2 | — | — | — | — |
| 263_loki_custom_stream_label (opus-4.6) 📄 | 4 | — | — | — | — |
| 264_es_numeric_severity_field (opus-4.6) 📄 | 7 | — | — | — | — |
| 265_es_timestamp_keyword_field (opus-4.6) 📄 | 4 | — | — | — | — |
| 266_es_schema_discovery (opus-4.6) 📄 | 3 | — | — | — | — |
| 267_bad_skill_resilience (opus-4.6) 📄 | 8 | — | — | — | — |
| 268_es_multi_quirk_consolidation (opus-4.6) 📄 | 10 | — | — | — | — |
| 269_es_skill_update_cross_investigation (opus-4.6) 📄 | 7 | — | — | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | — | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 1 | 1 | ±0% | — | — |
| Total (all, n=23) | 6.8 | 7.1 | — | — | — |
| Comparable (m=10, b=0) | 6.9 | 7.1 | ±0% | — | — |
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
📖 Legend
| Icon | Meaning |
|---|---|
| ✅ | The test was successful |
| ➖ | The test was skipped |
| The test failed but is known to be flaky or known to fail | |
| 🚧 | The test had a setup failure (not a code regression) |
| 🔧 | The test failed due to mock data issues (not a code regression) |
| 🚫 | The test was throttled by API rate limits/overload |
| ❌ | The test failed and should be fixed before merging the PR |
🔄 Re-run evals manually
⚠️ Warning:/evalcomments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.To test workflow changes, use the GitHub CLI or Actions UI instead:
gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/consolidated-skills-per-domain -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
tags: regression
Or with more options (one per line):
/eval
model: gpt-4o
tags: regression
id: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
tags: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
tags |
Pytest tags / markers (no default - runs all tests!) |
id |
Eval ID / pytest -k filter (use /list to see valid eval names) |
iterations |
Number of runs, max 10 |
branch |
Run evals on a different branch (for cross-branch comparison) |
Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.
Option 2: Trigger via GitHub Actions UI → "Run workflow"
Option 3: Add PR labels to include extra evals (applies to both automatic runs and /eval comments):
| Label | Effect |
|---|---|
evals-tag-<name> |
Run tests with tag <name> alongside regression |
evals-id-<name> |
Run a specific eval by test ID |
evals-model-<name> |
Override the model (use model list name, e.g. sonnet-4.5) |
Examples: evals-tag-easy, evals-id-09_crashpod, evals-model-sonnet-4.5
🏷️ Valid tags
benchmark, chain-of-causation, compaction, confluence, context_window, conversation_worker, coralogix, counting, database, datadog, datetime, db-connectors, easy, elasticsearch, embeds, fast, frontend, grafana, hard, images, integration, kafka, kubernetes, leaked-information, logs, loki, manual, mcp, medium, metrics, multi-cluster, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, skills, slackbot, storage, token-limit, toolset-limitation, traces, transparency, victorialogs
🤖 Valid models
deepseek-chat, deepseek-r1-reasoner, deepseek-reasoner, deepseek-v3.2-chat, gemini-3-flash-preview, gemini-3-pro-preview, gemini-3.1-pro-preview, gpt-4.1, gpt-5.2-high-reasoning, gpt-5.3-codex, gpt-5.4, gpt-5.5, haiku-4.5, kimi-2.5, kimi-2.5-openrouter, opus-4.5, opus-4.6, opus-4.7, opus-4.8, qwen-next-80B-instruct, qwen-next-80B-thinking, sonnet-4.5, sonnet-4.6
Commands: /eval · /rerun · /list
CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/consolidated-skills-per-domain -f markers=regression -f filter=
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds suggest-runbooks tooling, captures suggested memories during evals, writes/merges them as SKILL.md, supports closed-loop replay that loads those skills, and propagates memory/replay metrics into properties, Braintrust traces, and GitHub reporting. Includes many new fixtures exercising discovery and quirk consolidation. ChangesClosed-Loop Learning with Memory Capture and Replay for Holmes LLM Evals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:9d5a00dfb
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:9d5a00dfb me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:9d5a00dfb
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:9d5a00dfb
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:9d5a00dfb
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:9d5a00dfb me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:9d5a00dfb
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:9d5a00dfbPatch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:9d5a00dfb \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:9d5a00dfbRobusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:9d5a00dfb \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:9d5a00dfb |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/llm/fixtures/test_ask_holmes/61_exact_match_counting/test_case.yaml`:
- Around line 40-41: Add the missing include_tool_calls: true key to this
generic expected-output fixture so the test forces tool-call transparency;
specifically update the test_case.yaml to include include_tool_calls: true
alongside the existing memories_generated: false and expected_output entries so
the test cannot pass by hallucinating counts without tool-call evidence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 854369e8-8fc2-43d6-8ff4-622581bd4fd9
📒 Files selected for processing (40)
.goal_criteria.mdtests/llm/conftest.pytests/llm/fixtures/test_ask_holmes/09_crashpod/test_case.yamltests/llm/fixtures/test_ask_holmes/101_loki_historical_logs_pod_deleted/test_case.yamltests/llm/fixtures/test_ask_holmes/112_find_pvcs_by_uuid/test_case.yamltests/llm/fixtures/test_ask_holmes/12_job_crashing/test_case.yamltests/llm/fixtures/test_ask_holmes/176_network_policy_blocking_traffic_no_skills/test_case.yamltests/llm/fixtures/test_ask_holmes/227_count_configmaps_per_namespace/test_case.yamltests/llm/fixtures/test_ask_holmes/243_pod_names_contain_service/test_case.yamltests/llm/fixtures/test_ask_holmes/24_misconfigured_pvc/test_case.yamltests/llm/fixtures/test_ask_holmes/261_es_log_severity_field/test_case.yamltests/llm/fixtures/test_ask_holmes/261_es_log_severity_field/toolsets.yamltests/llm/fixtures/test_ask_holmes/262_prometheus_custom_kafka_metric_name/test_case.yamltests/llm/fixtures/test_ask_holmes/262_prometheus_custom_kafka_metric_name/toolsets.yamltests/llm/fixtures/test_ask_holmes/263_loki_custom_stream_label/app.pytests/llm/fixtures/test_ask_holmes/263_loki_custom_stream_label/deployment.yamltests/llm/fixtures/test_ask_holmes/263_loki_custom_stream_label/test_case.yamltests/llm/fixtures/test_ask_holmes/263_loki_custom_stream_label/toolsets.yamltests/llm/fixtures/test_ask_holmes/264_es_numeric_severity_field/test_case.yamltests/llm/fixtures/test_ask_holmes/264_es_numeric_severity_field/toolsets.yamltests/llm/fixtures/test_ask_holmes/265_es_timestamp_keyword_field/test_case.yamltests/llm/fixtures/test_ask_holmes/265_es_timestamp_keyword_field/toolsets.yamltests/llm/fixtures/test_ask_holmes/267_bad_skill_resilience/bad_skill/SKILL.mdtests/llm/fixtures/test_ask_holmes/267_bad_skill_resilience/test_case.yamltests/llm/fixtures/test_ask_holmes/267_bad_skill_resilience/toolsets.yamltests/llm/fixtures/test_ask_holmes/268_es_multi_quirk_consolidation/test_case.yamltests/llm/fixtures/test_ask_holmes/268_es_multi_quirk_consolidation/toolsets.yamltests/llm/fixtures/test_ask_holmes/43_current_datetime_from_prompt/test_case.yamltests/llm/fixtures/test_ask_holmes/51_logs_summarize_errors/test_case.yamltests/llm/fixtures/test_ask_holmes/61_exact_match_counting/test_case.yamltests/llm/test_ask_holmes.pytests/llm/utils/braintrust.pytests/llm/utils/property_manager.pytests/llm/utils/reporting/github_reporter.pytests/llm/utils/test_case_utils.pytests/llm/utils/test_results.pytests/llm/utils/test_toolset.pytests/llm/utils/tool_suggestions_config.pytests/test_test_results.pytests/test_tool_suggestions_config.py
Comment on lines
+40
to
+41
|
|
||
| memories_generated: false |
Contributor
There was a problem hiding this comment.
Add include_tool_calls: true for this generic expected-output fixture.
expected_output at Line 4 is intentionally broad (“either 6, 6 pods, etc.”). Without tool-call evidence, this can pass while hallucinating the count. Since Line 41 now enforces a strict memory behavior, this fixture should also enforce tool-call transparency.
💡 Suggested change
tags:
- counting
- easy
- regression
+include_tool_calls: true
memories_generated: falseAs per coding guidelines: "Use include_tool_calls: true in test_case.yaml when expected output is too generic to rule out hallucinations."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/llm/fixtures/test_ask_holmes/61_exact_match_counting/test_case.yaml`
around lines 40 - 41, Add the missing include_tool_calls: true key to this
generic expected-output fixture so the test forces tool-call transparency;
specifically update the test_case.yaml to include include_tool_calls: true
alongside the existing memories_generated: false and expected_output entries so
the test cannot pass by hallucinating counts without tool-call evidence.
Run 3 of the local eval surfaced a fixture bug: the test data hard-
coded ingest_ts to 2026-06-03 while the replay prompt ("how many ERROR
entries are there from today") let the agent resolve "today" to the
actual current date — which made index C return 0 entries (no logs on
the real today) even though the agent correctly fetched the
consolidated skill and used `ingest_ts` instead of `@timestamp`.
The skills mechanism worked perfectly in that run: agent emitted three
quirks all under skill_domain=elasticsearch, harness consolidated to
one SKILL.md, replay fetched that one skill and used the right field
for each of the three indices. Only the date arithmetic was off.
Fix: setup writes ingest_ts using the real $(date -u) for today and
yesterday so the data follows wall-clock time, and the primary prompt
no longer hardcodes "2026-06-03" — it asks for "today's date" so the
biased path still names @timestamp as the field to try but lets the
agent pick the current date. This makes the test stable regardless of
when CI runs it.
Signed-off-by: Claude <noreply@anthropic.com>
Run 4 surfaced the same severity_num ambiguity 264 has: bare integers
1-6 follow either syslog (lower = more severe) or winston/bunyan
(higher = more severe), and the agent reasonably inferred the wrong
one from the values alone. The data uses winston: 6=fatal, 5=error,
3=info — but the agent guessed syslog and reported 5 ERROR-equivalents
(severity_num=3) instead of 8 (severity_num in [5,6]).
Embedding the level name in the message text ("[ERROR] DB timeout",
"[FATAL] Disk full", "[INFO] healthcheck") gives any agent that
samples a single document an unambiguous cue. The discovery is still
real — the agent has to find that severity_num exists and figure out
the mapping — but now the data itself disambiguates, just like a real
customer's logs would. The skill emission will still capture the
discovered mapping under skill_domain=elasticsearch.
Signed-off-by: Claude <noreply@anthropic.com>
aantn
changed the title
The within-investigation consolidation 268 proved (multiple quirks emitted in one pass → one domain skill) is the easy case. The harder, and more realistic, case is: a saved domain skill from a prior investigation grows when a later investigation discovers a new quirk in the same data source. Without that, every customer would be stuck with whatever first-investigation skill was captured, and the catalog couldn't evolve. This commit adds: * `_parse_quirks_from_skill_md` parses a previously-written domain SKILL.md back into the same quirk-dict shape `write_memories_as_ skill_files` produces. Lenient parser — missing fields produce empty strings, legacy un-numbered headings still match. * `_existing_domain_skill_path` locates an existing domain skill under a target dir by matching `*quirks-for-querying-<domain>` so the numeric ordering prefix doesn't break lookup. * `write_memories_as_skill_files` now reads any pre-existing domain skill at the target path, parses its quirks, prepends them to the new emissions, dedupes by title, and rewrites the merged file. The same skill directory is reused so the filesystem layout stays stable as the skill grows. * `test_ask_holmes.py` replay block copies the fixture's `pre_loaded_skills_path` contents into the replay tempdir BEFORE calling the writer, so an eval can simulate a customer who already has a saved domain skill and the new emission has something to merge into. Test coverage: * Unit test: emitting a second quirk merges into the first instead of creating a parallel file or overwriting (verifies cross- investigation accumulation). * Unit test: re-emitting the same title is deduped. * Eval `269_es_skill_update_cross_investigation`: pre-loads a one- quirk ES domain skill (severity field for index A), asks about index B which requires a different quirk (ingest_ts timestamp). After the primary, the saved skill should contain BOTH quirks. Replay asks a cross-index question requiring both quirks; the agent must fetch the merged skill and use the right field for each index. Signed-off-by: Claude <noreply@anthropic.com>
Run 1 of 269 failed because the primary asks only about index B but expected_output listed answers for both A and B (which is what the replay asks). The framework checked the primary's answer against both expected elements and scored 0 because the primary correctly only addressed B. Adds `AskHolmesTestCase.expected_replay_output`: when set, the replay correctness check uses this list instead of the primary's `expected_output`. Falls back to the primary's `expected_output` when unset (existing evals are unaffected). 269 now declares: - expected_output: just B's answer (what primary asks) - expected_replay_output: both A and B answers (what replay asks after the merge made both quirks available in one skill) Signed-off-by: Claude <noreply@anthropic.com>
Eval 269 verifies cross-investigation merge of an emitted quirk into a pre-loaded domain skill. The primary pass already proves the merge via expected_skill_count: 1 (one consolidated file containing both quirks). On the 2-index replay question, the agent rationally chooses cheap mapping inspection over fetch_skill, which made the strict fetch_skill_called assertion fail even though the merge — the thing the eval is actually about — succeeded. Skill-load behavior is already covered by 261/262/264/265/268. Add an opt-out field (default True so existing evals are unchanged) and enable it on 269. Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_tool_suggestions_config.py (2)
156-157: ⚡ Quick winAdd type hints to the new test function signatures.
The new test functions are missing annotations for
tmp_pathand return type.💡 Suggested fix
+from pathlib import Path @@ -def test_write_memories_merges_into_existing_domain_skill(tmp_path): +def test_write_memories_merges_into_existing_domain_skill(tmp_path: Path) -> None: @@ -def test_write_memories_dedupes_existing_quirks(tmp_path): +def test_write_memories_dedupes_existing_quirks(tmp_path: Path) -> None:As per coding guidelines,
**/*.py: "Type hints required (mypy configuration in pyproject.toml)".Also applies to: 207-208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tool_suggestions_config.py` around lines 156 - 157, Add missing type hints to the new test function signatures: annotate the tmp_path parameter as pathlib.Path (or pytest.Path if your test suite uses that alias) and add a return type of None for functions such as test_write_memories_merges_into_existing_domain_skill (and the other new tests referenced around lines 207-208); update the function signatures to look like def test_write_memories_merges_into_existing_domain_skill(tmp_path: pathlib.Path) -> None: (and similarly for the other test names) to satisfy the project's mypy/type-hint requirements.
161-165: ⚡ Quick winUse module-level imports instead of importing inside test functions.
Both new tests import
osandtool_suggestions_configsymbols inside function scope. Move these imports to the file header.As per coding guidelines,
**/*.py: "Always place Python imports at the top of the file, not inside functions or methods".Also applies to: 211-215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tool_suggestions_config.py` around lines 161 - 165, Tests import os and the tool_suggestions_config symbols inside test functions; move those imports to the module top so they are module-level. Specifically, add "import os" and the imports for write_memories_as_skill_files and _parse_quirks_from_skill_md to the file header and remove the in-function imports, and likewise update the other test functions that currently import os or tool_suggestions_config symbols inside their bodies to use the header imports.tests/llm/utils/tool_suggestions_config.py (1)
372-372: ⚡ Quick winMove function-local imports to module scope in this Python module.
Imports at Line 372, Line 439, and Line 468–469 should be top-level to align with project conventions and avoid repeated local import paths.
♻️ Suggested refactor
import json import logging +import os +import re +from collections import OrderedDict from typing import Any, Dict, List, Optional @@ def _slugify(text: str) -> str: @@ - import re - @@ def _normalize_skill_domain(raw: Optional[str]) -> str: @@ - import re - @@ def _parse_quirks_from_skill_md(skill_md_path: str) -> List[Dict[str, Any]]: @@ - import re - @@ def _existing_domain_skill_path( @@ - import os - @@ def write_memories_as_skill_files( @@ - import os - from collections import OrderedDictAs per coding guidelines,
**/*.py: "Always place Python imports at the top of the file, not inside functions or methods".Also applies to: 439-440, 468-469
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/llm/utils/tool_suggestions_config.py` at line 372, Move the function-local imports to module scope: find every inline import statement (e.g., "import re" and the other imports currently declared inside functions) and hoist them to the top of the file, then remove the duplicate local imports inside the functions; ensure you import the same symbols at module-level so functions using them (search for occurrences of "import re" and other inline imports) keep working and adjust any conditional/circular cases if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/llm/utils/tool_suggestions_config.py`:
- Around line 399-413: The parser _parse_quirks_from_skill_md currently includes
the inter-entry markdown separator (e.g. '---') in the sliced section so
_extract() can return values ending with '---'; fix by trimming trailing
markdown separator lines from the extracted value: after mm.group(1).strip()
(inside _extract) remove any trailing separator lines (e.g. lines that consist
of three or more hyphens and surrounding whitespace/newlines) before returning;
alternatively, adjust the section end logic that builds `section` (using
`matches`/`start`/`end`) to exclude adjacent separator-only lines so _extract
returns clean field text. Ensure references to _extract, section, matches and
_parse_quirks_from_skill_md are updated accordingly.
---
Nitpick comments:
In `@tests/llm/utils/tool_suggestions_config.py`:
- Line 372: Move the function-local imports to module scope: find every inline
import statement (e.g., "import re" and the other imports currently declared
inside functions) and hoist them to the top of the file, then remove the
duplicate local imports inside the functions; ensure you import the same symbols
at module-level so functions using them (search for occurrences of "import re"
and other inline imports) keep working and adjust any conditional/circular cases
if needed.
In `@tests/test_tool_suggestions_config.py`:
- Around line 156-157: Add missing type hints to the new test function
signatures: annotate the tmp_path parameter as pathlib.Path (or pytest.Path if
your test suite uses that alias) and add a return type of None for functions
such as test_write_memories_merges_into_existing_domain_skill (and the other new
tests referenced around lines 207-208); update the function signatures to look
like def test_write_memories_merges_into_existing_domain_skill(tmp_path:
pathlib.Path) -> None: (and similarly for the other test names) to satisfy the
project's mypy/type-hint requirements.
- Around line 161-165: Tests import os and the tool_suggestions_config symbols
inside test functions; move those imports to the module top so they are
module-level. Specifically, add "import os" and the imports for
write_memories_as_skill_files and _parse_quirks_from_skill_md to the file header
and remove the in-function imports, and likewise update the other test functions
that currently import os or tool_suggestions_config symbols inside their bodies
to use the header imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1090871-740c-413e-a73e-980f481a721d
📒 Files selected for processing (7)
tests/llm/fixtures/test_ask_holmes/269_es_skill_update_cross_investigation/prior_quirks/quirks-for-querying-elasticsearch/SKILL.mdtests/llm/fixtures/test_ask_holmes/269_es_skill_update_cross_investigation/test_case.yamltests/llm/fixtures/test_ask_holmes/269_es_skill_update_cross_investigation/toolsets.yamltests/llm/test_ask_holmes.pytests/llm/utils/test_case_utils.pytests/llm/utils/tool_suggestions_config.pytests/test_tool_suggestions_config.py
✅ Files skipped from review due to trivial changes (2)
- tests/llm/fixtures/test_ask_holmes/269_es_skill_update_cross_investigation/toolsets.yaml
- tests/llm/fixtures/test_ask_holmes/269_es_skill_update_cross_investigation/prior_quirks/quirks-for-querying-elasticsearch/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/llm/utils/test_case_utils.py
- tests/llm/test_ask_holmes.py
Comment on lines
+399
to
+413
| end = matches[idx + 1].start() if idx + 1 < len(matches) else len(body) | ||
| section = body[start:end] | ||
|
|
||
| def _extract(label: str) -> str: | ||
| # Pulls the text between `**<label>:**` and the next `**...**:` | ||
| # bold-label or end of section. Tolerates inline-value style | ||
| # ("**When to use:** text") and block-value style. | ||
| pat = re.compile( | ||
| rf"\*\*{re.escape(label)}:?\*\*\s*(.*?)(?=\n\*\*[^*]+:?\*\*|\Z)", | ||
| re.DOTALL, | ||
| ) | ||
| mm = pat.search(section) | ||
| if not mm: | ||
| return "" | ||
| return mm.group(1).strip().lstrip("-").strip() |
Contributor
There was a problem hiding this comment.
_parse_quirks_from_skill_md can persist markdown separators into parsed quirk fields.
Line 399–413 parses each section up to the next heading, but that slice includes the inter-entry --- separator. Because _extract(...) reads to section end, values like why_env_specific can become "…\n\n---", and then get rewritten back into SKILL.md on merge.
💡 Suggested fix
- section = body[start:end]
+ section = re.sub(r"\n---\s*$", "", body[start:end].strip(), flags=re.MULTILINE)
def _extract(label: str) -> str:
@@
pat = re.compile(
- rf"\*\*{re.escape(label)}:?\*\*\s*(.*?)(?=\n\*\*[^*]+:?\*\*|\Z)",
+ rf"\*\*{re.escape(label)}:?\*\*\s*(.*?)(?=\n\*\*[^*]+:?\*\*|\n---\s*\n|\Z)",
re.DOTALL,
)🧰 Tools
🪛 Ruff (0.15.15)
[warning] 410-410: Function definition does not bind loop variable section
(B023)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/llm/utils/tool_suggestions_config.py` around lines 399 - 413, The
parser _parse_quirks_from_skill_md currently includes the inter-entry markdown
separator (e.g. '---') in the sliced section so _extract() can return values
ending with '---'; fix by trimming trailing markdown separator lines from the
extracted value: after mm.group(1).strip() (inside _extract) remove any trailing
separator lines (e.g. lines that consist of three or more hyphens and
surrounding whitespace/newlines) before returning; alternatively, adjust the
section end logic that builds `section` (using `matches`/`start`/`end`) to
exclude adjacent separator-only lines so _extract returns clean field text.
Ensure references to _extract, section, matches and _parse_quirks_from_skill_md
are updated accordingly.
The CI run on PR #2128 showed 261-265 all regressed to 0% — the agent on replay ignored the saved skill and rediscovered the quirk from scratch. Two root causes: 1. The replay-time skill written to a tempdir was being passed to SkillsToolset (so fetch_skill could load it by name) but NOT to the SkillCatalog loaded for the prompt's "Skill Catalog" section. The agent only saw the bare skill name in fetch_skill's param list, never the description. Fix: thread additional_skill_paths into the load_skill_catalog call in ask_holmes so the description block lands in the prompt. 2. The consolidated skill's description framed itself as a pre-flight optimization ("fetch BEFORE issuing the first query ... to skip wrong-call recovery"). The base prompt explicitly gates fetches with "only fetch skills that clearly match — do not fetch speculatively", so a soft hedge doesn't trip the gate. Rewrote to a deterministic, rule-based, aggressively imperative trigger: "MANDATORY pre-read for ANY investigation that will touch <domain> ... any user question that will touch <domain> is a clear match — fetch it, do not skip." Also fixed 264's data setup: the numeric severity_num convention was ambiguous to the model (syslog 3=ERROR/0=EMERG vs winston 5=ERROR/6=FATAL). Embedded [ERROR]/[FATAL]/[WARN]/[INFO] tags in the msg field so a single sample doc disambiguates — same fix that worked for 268. Verified locally: 261, 264, 265 all pass at 100% with the fix. 262 and 263 need K8s and will be validated by CI. Signed-off-by: Claude <noreply@anthropic.com>
The GitHub Actions eval report had one "Memories" column carrying double duty: emitted-suggest_runbooks count on primary rows and "skill ✓/✗" on replay rows. That conflated two different things and made it impossible to see at a glance whether a primary investigation ALSO consulted any pre-loaded skills. Split into two columns: - Skill Generated: count of suggest_runbooks emissions (always "—" on replay rows since suggest_runbooks isn't injected on replay) - Skills Read: count of fetch_skill calls (tracked on both primary and replay) Moved both to sit immediately before Compactions so the skills-related counters cluster together at the end of the row. Track new user properties skills_read_count and replay_skills_read_count to source the new column. replay_skill_loaded boolean is kept for the existing assert in test_ask_holmes. Signed-off-by: Claude <noreply@anthropic.com>
The 'net win summary' block had an explanatory line ('Negative delta =
replay was cheaper...') and bold/imperative framing. Per feedback, just
emit the raw counts and the mean delta inside a collapsed <details>
block — readers can interpret on their own.
Signed-off-by: Claude <noreply@anthropic.com>
Conflicts: kept both sides everywhere — - test_ask_holmes.py: additional_skill_paths + enable_todo / prompt overrides - test_case_utils.py: skills fields + enable_todo - test_toolset.py: additional_skill_paths + enable_todo - github_reporter.py: Skill Generated/Skills Read columns + master's new Denied commands column (order: ...Reasoning | Skill Generated | Skills Read | Compactions | Denied commands | Src) - braintrust.py: restore List import dropped by auto-merge Signed-off-by: Claude <noreply@anthropic.com>
…ilures The capture rule only fired on wrong->right corrections and explicitly forbade emission when every call succeeded. That misses the larger cost the mechanism exists to eliminate: in every fresh chat the agent re-learns a data source's schema (mapping inspections, label/metric/index listings, document sampling) before it can query - none of which fails, all of which repeats. Tool interface: - New required `kind` field: "correction" (existing wrong->right pair) or "discovery" (stable env facts learned through successful exploration). - failed_call documented as empty-string for discovery (strict tool mode force-requires all properties, so optionality is by convention). - System prompt: capture-discovery patterns, plus a principled exclusion - schemas/naming conventions/addressing are stable and capturable; Kubernetes resource inventory and counts are transient and are not. Skill rendering: - Discovery entries render "Environment facts and call shape" instead of the failed/working pair; kind round-trips through the merge parser structurally (entry without a failed-call section = discovery). - Importance now rendered and parsed back (merges no longer reset it). - Titles sanitized to one line so `## N. title` headings and the merge parser's entry splitting can't be broken by multi-line LLM titles. - New body DIRECTIVE: entries are verified facts, query directly, do not re-verify unless a query contradicts them. Needed because the generic fetch_skill wrapper says skill contents are "DIRECTIONS not ACTUAL RESULTS" / "just an EXAMPLE", which made the replay agent re-inspect the mapping to confirm the saved schema. Harness: - replay_forbidden_tools eval field: tools that must NOT appear on replay, proving the captured skill actually obviated the exploration it encodes. Eval: - 266_es_schema_discovery: index with compact custom fields (lvl/txt/app/ ts); primary asks for field inventory + ERROR count so exploration happens with zero failures; replay asks naturally and must answer via the skill without calling elasticsearch_mappings. Validated locally on opus-4.6: 266 passes 100% (discovery loop), 261 passes 100% (correction loop unaffected by the prompt rewrite). Unit tests: 13 passing (3 new for discovery rendering/round-trip/title sanitization). Signed-off-by: Claude <noreply@anthropic.com>
aantn
changed the title
The tool saves skills, not runbooks — the old name forced an awkward
counter-instruction in its own prompt ('Never refer to these as
runbooks'), now removed. The name is not a holmes-core or frontend
contract (zero references outside the eval harness), so the rename is
purely mechanical: wire name, constants, function names, fixture
comments.
Validated: 18 unit tests pass; eval 261 passes 100% end-to-end with the
renamed tool (emission + replay).
Signed-off-by: Claude <noreply@anthropic.com>
This branch has not been deployed
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.
Summary
Closed-loop "skills/memories" mechanism for HolmesGPT. It's very common for the agent to do trial-and-error and schema rediscovery when touching a data source in a fresh chat: it re-learns how to query it, what the field names are, what the labels/metrics are called. This PR lets the agent capture those learnings once, save them as a skill, and have every future investigation skip the rediscovery.
Two kinds of learnings are captured (declared per entry via a required
kindfield on the tool schema):kind: correction— the agent called a tool the way a fresh LLM would default to, got an empty/wrong result, and succeeded only after an env-specific parameter change (custom label scheme, renamed metric prefix, non-standard log field, alias-only index...). The wrong→right pair is saved so the next run skips the failed attempt.kind: discovery— nothing failed, but the agent spent exploratory calls (mapping/schema inspection, label/metric/index listing, document sampling) learning stable environment facts before it could issue the real query. The facts + direct call shape are saved so the next run queries immediately. Guardrail: schemas and naming conventions are stable and capturable; Kubernetes resource inventory/counts are transient and excluded.Design
skill_domain(elasticsearch,loki,prometheus, ...). The harness consolidates all entries sharing a domain into onequirks-for-querying-<domain>/SKILL.mdwith numbered entries — so the skill catalog stays small at customer scale and a single fetch surfaces every known quirk for that source.skill_domain. (The first CI run showed soft descriptions never trip the base prompt's "only fetch skills that clearly match" gate.)fetch_skillwrapper tells agents skill contents are "DIRECTIONS not ACTUAL RESULTS"; right for runbooks, wrong for saved facts — it made replay agents re-inspect mappings to "confirm" the skill. The generated body now states entries are verified facts: query directly, re-verify only on contradiction.Closed-loop eval validation (all passing locally on opus-4.6)
Each eval runs the primary investigation, captures emissions, writes the consolidated skill, then replays a naturally-phrased question with the skill loaded and asserts the agent fetched it and answered correctly.
261/264/265(ES),262(Prometheus),263(Loki): forced-correction captures, one per data source quirk266_es_schema_discovery: discovery without failure — schema learned via mapping inspection, zero failed calls; replay must answer without callingelasticsearch_mappingsagain (newreplay_forbidden_toolsassertion proves the skill eliminated the rediscovery)267_bad_skill_resilience: pre-loaded misleading skill; agent must recover and still answer correctly268_es_multi_quirk_consolidation: 3 quirks in one investigation → 1 skill; replay −44% cost / −58% tokens269_es_skill_update_cross_investigation: new emission merges into a pre-existing domain skill (cross-investigation accumulation)Harness/test-framework additions
rerun_with_memory,replay_user_prompt,expected_replay_output,pre_loaded_skills_path,expected_skill_count,require_skill_load_on_replay,replay_forbidden_toolsfields on test casesSkill Generated/Skills Readcolumns,[replay]comparison rows, collapsed skills-stats sectionhttps://claude.ai/code/session_01NkgEXcoZhJD498VPmsYXmb