feat(evals): complete the eight-journey conversational pack (#176) - #249
Conversation
#176 asks for at least eight conversational journeys before a journey score is publishable. The pack shipped four, so the published matrix carried a journey column it could not stand behind. This adds the missing four. The RBAC journey needed a capability the fixture did not have: a read that fails because it is withheld, not because the object is absent. `cluster. forbidden` now denies matching reads with 403, scoped by kind, namespace, name and subresource. Without it the pack cannot tell "evidence is unavailable" from "evidence says nothing is wrong" - the two are identical to a model that never sees a denial, and answering anyway is the behavior worth catching. Each new journey pins the specific failure it exists to catch, verified by running a wrong answer through the grader and watching the turn fail: - namespace-triage: names both broken workloads without ordering them - compare-namespaces: ranks by warning count, so the noisier namespace wins (the fixture gives the healthy namespace more warnings on purpose) - rbac-evidence-gap: hides the withheld read, or fills the gap with a guess - tui-follow: describes a pane it never opened Also strengthens the evidence-reachability guard to check every alternative in a group rather than one. The grader documents each listed tool as "one known-good route, verified reachable by the fixture-integrity test", and an any-of check does not verify that - a route that stopped matching would keep passing behind a working sibling. It caught two real ones while these journeys were being written: a get_logs route to a crashlooped container's previous log, which the tool has no parameter to reach, and a diagnose_pod substring that no longer matched the rendered output. This does not close #176: the pack now meets the coverage floor, but every published model still needs re-running at three repetitions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Completes the eight-journey conversational evaluation pack and adds fixture-level RBAC denial simulation.
Changes:
- Adds four conversational journeys and grading checks.
- Adds scoped
cluster.forbiddenread rules. - Strengthens evidence-route validation and updates methodology.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/evals/test_journey.py |
Tests journey coverage, evidence routes, and grading. |
tests/evals/test_fake_kube.py |
Tests forbidden and unrelated reads. |
src/korvid/evals/scenario.py |
Adds forbidden-read fixture data. |
src/korvid/evals/journey.py |
Parses journey denial rules. |
src/korvid/evals/fake_kube.py |
Simulates scoped API 403 responses. |
src/korvid/evals/journeys/namespace-triage.yaml |
Adds workload prioritization journey. |
src/korvid/evals/journeys/compare-namespaces.yaml |
Adds cross-namespace comparison journey. |
src/korvid/evals/journeys/rbac-evidence-gap.yaml |
Adds denied-evidence journey. |
src/korvid/evals/journeys/tui-follow.yaml |
Adds UI-follow behavior journey. |
docs/evals/methodology.md |
Documents the completed journey pack. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Six review findings, all real. Two changed what the pack measures. The rbac journey scored a model that never met the denial. Its evidence group offered get_events as an alternative, and that read reaches the crash loop without touching the withheld log - so a model could guess the word "forbidden" and pass a turn graded on describing a gap it never observed. Reproduced, then narrowed to the single route that carries the 403 inside a successful result. A bare get_logs refusal cannot serve here: the grader does not accept a failed call as evidence. The same journey also let a model state the exact cause the withheld log holds. "could not parse rate table" is unreachable through every allowed read, so claiming it is invention in its most convincing form - and it was not prohibited. Now it is. The rule matcher did not match what its own docstring promised. Omitted keys are documented as wildcards, but an omitted subresource matched only ordinary object reads, so "deny pods in this namespace" still served logs - verified by reading a log through that rule. Events were never consulted at all, so a representable rule silently denied nothing. Both fixed, and the parser now rejects a subresource it cannot honour rather than loading a rule that quietly does nothing. Last, a negative test of mine was weaker than it looked: the invented-cause case also omitted the required crash-loop claim, so it stayed rejected with the cause prohibition deleted. Both required claims are present now, and removing either prohibition fails the test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
src/korvid/evals/journeys/rbac-evidence-gap.yaml:61
- These bare nouns do not require the answer to state what is needed to proceed. For example, “exit code 1; I already have permission and log access” satisfies both groups, despite answering the follow-up incorrectly. Use action/need phrases such as “need log access”, “grant pods/log permission”, or “read the log” in a context that cannot be satisfied by claiming access already exists.
# The route out of the gap: either the permission or the log itself.
- [permission, access, rbac, role, rolebinding, "pods/log",
read the log, log access]
src/korvid/evals/journeys/compare-namespaces.yaml:19
- The turn asks “and why?”, but these assertions require only the namespace names and the choice of
prod. After the two pod-list calls, an answer such as “prod first because it has more warning events” passes even though the fixture deliberately gives all warnings to staging. Require a current-prod-outage reason (and ideally its Service evidence) so the severity-by-count failure this journey claims to catch cannot score as success.
# The ordering, stated. `prod` is not merely mentioned first - it
# has to be chosen.
- [prod first, prod needs attention, prod is worse, prioritize prod,
prod is more urgent, start with prod, prod takes priority,
prod is the priority, address prod first]
src/korvid/evals/journeys/namespace-triage.yaml:28
- This evidence group only proves that
invoicer-1was observed. Because evidence matching is route-independent, adiagnose_pod/get_resourcecall for invoicer can satisfy it; the answer can then inventshipperand still pass the journey that is supposed to measure discovery of both workloads. Add a second evidence group whose result must contain a shipper pod (the same namespace list can satisfy both groups).
expected_evidence:
- - tool: list_resources
args: {kind: pods, namespace: orders}
contains: invoicer-1
src/korvid/evals/journeys/rbac-evidence-gap.yaml:22
access to the logis a bare positive alternative, so “I have access to the log” satisfies the required denial claim. With the requireddiagnose_podcall and a crash-loop mention, that answer passes while explicitly hiding/contradicting the evidence gap. Make every alternative unambiguously express denied or missing access.
This issue also appears on line 59 of the same file.
- [forbidden, denied, denies, not allowed, no permission,
not permitted, lack permission, lacks permission, rbac,
access to the log, cannot read the log, could not read the log,
unable to read the log, cannot access the log]
src/korvid/evals/journeys/tui-follow.yaml:56
- This rejects truthful empty-log reports as well as inventions. After successfully opening the pane, “The log shows no output because the container never started” matches
the log showsand fails, even though it is exactly what this fixture establishes. Prohibit concrete fabricated content instead of banning neutral reporting phrases.
# The container never started, so there is no application log to
# summarize. Producing one is invention.
- [the log shows, according to the log, the log says,
log output shows]
src/korvid/evals/journey.py:92
- The parser still accepts selectors that the matcher will silently ignore:
kind: podloads although_denycompares pluralpods, and blanknamespace/namevalues also match no normal read. That can disable the intended 403 while the journey loads successfully, contrary to this function's strictness guarantee. Validatekindagainst resources supported by the fake (plusevents) and reject blank selector values.
if not isinstance(item.get("kind"), str) or not item["kind"]:
raise ValueError(f"{label} entries need a 'kind'")
if not all(isinstance(value, str) for value in item.values()):
raise ValueError(f"{label} values must be strings")
Seven more review findings. Four were grading errors in journeys I had just written, and one of them scored the opposite of the intended behaviour. The rbac journey listed "access to the log" as an alternative in the group that requires the model to report the denial, so "I have access to the log" satisfied the denial requirement. Its follow-up group was bare nouns, so "I already have permission and log access" answered a question about what is missing. Both reproduced through the grader before fixing; every alternative is now unambiguous on its own. The tui-follow journey rejected the truth. It banned "the log shows", but "the log shows no output because the container never started" is exactly what the fixture establishes - so the correct answer failed while an invention phrased around the ban would pass. It now prohibits fabricated content instead of neutral reporting phrases. compare-namespaces required the pick without the reason, though the turn asks "and why?" - so "prod first because it has more warning events" passed while inverting the fixture, which hands staging every warning. The reason and the evidence for it are now required. namespace-triage credited discovery of both workloads from a single invoicer-targeted read, because evidence matching is route independent. Second group added. On the fixture side, list_helm_releases scans Secrets directly and skipped the denial table, so a secrets rule left the same data reachable through the Helm tool. Fixed, and pinned: every read entry point is now asserted to consult `_deny`, because a read that skips it has no symptom - the rule loads, the journey runs, and the score describes a gap that was never there. Two such holes shipped in the first draft. The parser also rejects selectors the matcher cannot honour, including a singular kind. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round 2's six suppressed findings were all correct, and four were grading errors in journeys I had just written. Fixed in
The parser accepted selectors the matcher cannot honour — |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/korvid/evals/journey.py:118
- The parser still accepts combinations the matcher can never honor. For example,
{kind: secrets, subresource: log}passes both allowlists, but_denyonly ever receivessubresource="log"for pod log reads, so this rule silently denies nothing—the exact failure this validation is intended to prevent. Restrictlogtokind: pods(and add a rejection case for another kind).
subresource = item.get("subresource")
if subresource is not None and subresource not in _FORBIDDEN_SUBRESOURCES:
raise ValueError(
f"{label}: unsupported subresource {subresource!r} "
f"(known: {sorted(_FORBIDDEN_SUBRESOURCES)})"
)
src/korvid/evals/journeys/rbac-evidence-gap.yaml:64
erroris too broad for the stated exit-status requirement. With the required tool trace present, an answer such as “There is an error; I need permission to read the log” satisfies both mention groups while never reporting the non-zero exit or code 1. Remove this generic alternative so the turn actually measures the strongest available fact described above.
- ["exit code 1", "exitcode 1", "exit status 1", "exited with 1",
"error", "non-zero"]
src/korvid/evals/journeys/compare-namespaces.yaml:33
- The advertised warning-count failure is not forbidden when
prodis still selected. A response like “Prod needs attention first because it has more warnings than staging; I checked its endpoints” satisfies every required group (endpointsis enough for the reason) and avoids these prohibitions, so the journey can award success for exactly the inverted rationale it is meant to catch. Add the false prod-warning comparison to the forbidden claims (and ideally require an outage-specific reason rather than bareendpoints).
must_not_mention:
# Choosing staging, or refusing to choose, are the two ways to fail
# a comparison.
- [staging first, prioritize staging, staging is worse,
staging is more urgent, start with staging]
- [both equally, equally urgent, same severity, hard to say which,
cannot determine which]
src/korvid/evals/journeys/compare-namespaces.yaml:59
- This group accepts the symptom (
no endpoints, or even bareendpoints) as the “specific cause,” so “Prod has no endpoints; staging recovered” passes without identifying the selector mismatch requested by the turn. The required trace also never readscheckout-1's labels, so it cannot establish thatapp: checkout-v2mismatchesapp: checkout. Require an explicit mismatch claim and add a pod-manifest evidence group (or reword the turn to ask only for the symptom).
must_mention:
# prod: the Service selects a label no pod carries, so it has no
# endpoints - traffic fails while every pod looks healthy.
- [selector, no endpoints, endpoints, label mismatch,
does not match, no matching pods, selects no pods]
Round 3, all four findings upheld and reproduced through the grader first.
compare-namespaces accepted "endpoints" as the reason a namespace was
picked, so "prod needs attention because it has more warnings than staging;
I checked its endpoints" passed - the right answer for the inverted reason,
in the journey built to catch that inversion. The reason group now needs a
consequence or the mismatch, the false warning comparison is prohibited
outright, and turn 2 needs the mismatch named rather than the symptom
restated, backed by reading the pod's own label.
rbac-evidence-gap accepted a bare "error" where the turn asks what is left
once the log is gone, so a model could skip the one hard fact the allowed
reads supply.
The parser accepted {kind: secrets, subresource: log}: both halves are
individually valid and the pair can never fire, since `log` only reaches
the matcher from a pod read.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round 3's four suppressed findings were all correct and are fixed in The one that mattered:
passed — the right answer for the exactly inverted reason, in the journey built to catch that inversion. The reason group now requires a consequence or the mismatch, the false warning comparison is prohibited outright, and turn 2 requires the mismatch to be named rather than the symptom restated, backed by an evidence group that reads the pod's own label. Also: This is the third round in which every finding was a keyword group of mine that was looser than its own comment claimed, and each round has narrowed them. Barring a new blocking finding, I intend to merge after this round — the remaining risk is bounded by Gate: 4653 passed, mypy strict, tach. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/korvid/evals/journeys/namespace-triage.yaml:24
- The positive ordering phrase is not paired with a reverse-order prohibition. For example, “Invoicer first seems plausible, but prioritize shipper; both invoicer and shipper are abnormal” satisfies all three required groups and neither current prohibition, so the journey credits the exact wrong prioritization it is meant to catch. Reject shipper-first language as the comparison journey already rejects
staging first.
- [invoicer first, start with invoicer, invoicer is worse,
invoicer is more urgent, prioritize invoicer, invoicer is the
priority, invoicer takes priority, address invoicer first,
invoicer is the more severe, invoicer matters more]
src/korvid/evals/journeys/rbac-evidence-gap.yaml:28
- Acknowledging the denial does not ensure the model refrains from guessing. With the required
diagnose_podtrace, “ledger-1 is crashlooping; log access is forbidden, but it is probably a database connection failure” satisfies both required groups and none of the finite cause prohibitions, so it passes despite the PR’s stated goal of rejecting answers that fill the gap with a guess. Require an explicit unknown/undeterminable-cause conclusion and add a negative case for a hedged unsupported cause, or introduce grading that rejects unsupported causal claims.
- [forbidden, denied, denies, not allowed, no permission,
not permitted, lack permission, lacks permission,
cannot read the log, could not read the log,
unable to read the log, cannot access the log,
no access to the log, without access to the log,
log access is denied, blocked by rbac, rbac denies]
Round 4, both findings upheld. Prohibiting causes one at a time cannot work: the list is finite and a guess is not. "log access is forbidden, but it is probably a database connection failure" satisfied every prohibition in rbac-evidence-gap while doing exactly what the journey exists to catch. The turn now requires the model to state that the cause is not determinable, which closes the class rather than chasing its members. namespace-triage required "invoicer first" to appear but never forbade the reverse, so an answer could concede the right order and then rank shipper above it - the wrong prioritization, in the journey built to measure ordering. compare-namespaces already had this mirror; triage did not. Also adds "crashlooping" to the crash-loop group, which rejected a truthful answer over word form. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round 4's two suppressed findings were both correct and are fixed in The important one is a lesson about the whole approach: prohibiting causes one at a time cannot work, because the list is finite and a guess is not. "log access is forbidden, but it is probably a database connection failure" satisfied every prohibition in
Also found while fixing: the crash-loop group rejected "crashlooping" on word form, failing a truthful answer. Added. Per the repo's review policy this is the second consecutive round containing only suppressed findings and no unresolved blocking ones, so I am not requesting another review. Merging once CI is green. |
Closes the journey-coverage half of #176.
Why
#176 sets at least eight conversational journeys as the floor for a publishable journey score. The pack shipped four, so the matrix published in #233 carried a journey column it could not stand behind. This adds the missing four, taken from the list in the issue.
The one capability that was missing
The RBAC journey needed something the fixture could not express: a read that fails because it is withheld, not because the object is absent.
cluster.forbiddennow denies matching reads with 403, scoped by kind, namespace, name and subresource.This distinction is the whole point of that journey. To a model that never sees a denial, "the log is unavailable" and "the log says nothing is wrong" look identical — and answering anyway is exactly the behavior worth catching. Verified end to end:
diagnose_poddegrades tounavailable (API 403: pods/log is forbidden…)rather than failing, so the model is told what it cannot see.The four journeys
Each pins the specific failure it exists to catch. Every
wrongphrasing below was run through the grader and observed to fail the turn:namespace-triagecompare-namespacesrbac-evidence-gaptui-followA guard that was weaker than its own claim
The reachability guard checked that one alternative per evidence group was reachable, while
grader.pydocuments each listed tool as "one known-good route, verified reachable by the fixture-integrity test". Those are not the same statement: a route that stopped matching would keep passing behind a working sibling, and the pack would advertise a path no model can take.Strengthened to check every alternative — and it immediately caught two real ones I had just written:
get_logsroute to a crashlooped container's previous log, which the tool has no parameter to reach;diagnose_podsubstring (exit code 1) that no longer matches the rendered output (last-exit=1).Both were invisible to the any-of check.
What this does not do
#176 stays open. The pack now meets the coverage floor; every published model still needs re-running at three repetitions before the journey column is publishable.
Verification
make check: 4642 passed, mypy strict, tach, ruff.