Pin every journey turn in both directions, and stop crediting "it is fine" - #252
Conversation
There was a problem hiding this comment.
Pull request overview
Adds bidirectional grading pins for every bundled journey turn and strengthens detection of misleading all-clear claims.
Changes:
- Adds accepting/rejecting journey fixtures and coverage enforcement.
- Introduces all-clear predicate handling in the grader.
- Tightens grading rules for several journeys.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/korvid/evals/grader.py |
Adds all-clear predicate detection. |
src/korvid/evals/journeys/namespace-triage.yaml |
Requires configuration absence. |
src/korvid/evals/journeys/rollout-owner-chain.yaml |
Rejects false scale-down claims. |
src/korvid/evals/journeys/triage-and-correct.yaml |
Strengthens ordering, attribution, and remedy rules. |
tests/evals/test_grader.py |
Tests exculpatory predicates and scope. |
tests/evals/test_journey.py |
Pins both grading directions for every turn. |
docs/evals/methodology.md |
Documents bidirectional pinning. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/korvid/evals/grader.py:164
workingis not unconditionally all-clear: a correct diagnosis such as “the liveness probe is working too slowly and timing out” is currently exculpated as soon asworkingis seen, so the requiredliveness probegroup becomes missing. Account for fault qualifiers after this token, or narrow this set so valid failure claims are not suppressed.
"passing",
"succeeding",
"working",
"good",
src/korvid/evals/grader.py:378
- Disabling exculpation for the whole scenario also disables it for
must_not_mention, where it must remain active. For a healthy scenario that requireshealthyand forbidsimage pull, “Everything is healthy; the image pull looks normal” is now rejected as a positive image-pull diagnosis, contrary to the symmetric polarity contract. Disable exculpation only while evaluating required groups in negative controls; forbidden groups should always treat all-clear predicates as rule-outs.
# A scenario with no fault is graded on an all-clear, so "the endpoints
# are healthy" is its answer rather than a way of dodging the question.
exculpation = scenario.root_cause != "none"
def claims(keyword: str) -> bool:
return _mentions_positively(keyword, answer_tokens, clause_ids, exculpation=exculpation)
tests/evals/test_journey.py:684
- This coverage check can pass even though the phrase test does not use each journey's actual polarity.
_turn_scenario()hardcodesroot_cause="r", so the newly addedhealthy-stopcases are graded as fault scenarios even though production loads them withroot_cause: none; that now changes exculpation behavior. Construct the test scenario withjourney.root_causeso these pins exercise the real grading configuration.
def test_every_journey_turn_pins_an_accepting_and_a_rejecting_answer() -> None:
src/korvid/evals/journeys/triage-and-correct.yaml:21
- The reverse list omits the exact mirror of the accepted
checkout should be inspected firstwording. Consequently, “checkout should be inspected first, but payments should be inspected first instead” still satisfies every required group without hitting this prohibition. Add the symmetric phrasing and pin it as a rejecting case.
- [payments first, prioritize payments, start with payments,
inspect payments first, payments is worse,
payments is more urgent, payments takes priority]
src/korvid/evals/journeys/triage-and-correct.yaml:51
- This still permits the wrong-subject attribution through an unlisted grammar. For example, “payments-1 is failing; checkout-1 suffers an authentication failure” satisfies
[payments]and[authentication]but matches none of thesecheckout is/hasphrases. Bind the required cause to payments (or otherwise cover checkout-bound authentication claims) and pin this form, rather than only prohibiting one sentence shape.
- [checkout is the one, checkout-1 is the one, checkout is the cause,
checkout-1 is the cause, cause is checkout, blame checkout,
checkout is unauthorized, checkout-1 is unauthorized,
checkout has the registry, checkout-1 has the registry,
checkout has the unauthorized, checkout-1 has the unauthorized,
src/korvid/evals/journeys/namespace-triage.yaml:60
- Common contraction forms are not accepted here: tokenization turns “DSN isn't set” into
dsn isn t set, which does not matchdsn is not set. That makes a correct diagnosis fail despite expressing the required subject-bound absence. Include the contraction variants and pin one as accepting.
- [dsn is missing, missing dsn, dsn is not set, dsn is unset,
database_dsn is missing, missing database_dsn,
database_dsn is not set, database url is missing,
|
Round 2: six suppressed findings, all six upheld and fixed in Polarity is not symmetric. Disabling exculpation for a negative control disabled it for The pins were not exercising production. The other four are the recurring lesson that a list cannot bound a sentence:
The attribution fix is the interesting one: instead of enumerating ways to blame the wrong pod, the required cause is now bound to payments, which closes the class. That immediately over-narrowed — "payments has an unauthorized ..." missed by one token and broke Every reviewed phrasing is pinned. Gate green: 4744 passed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
src/korvid/evals/grader.py:388
- Applying exculpation to every forbidden alternative also suppresses alternatives that already encode a fault. For the bundled
healthy-service-endpointsscenario,"Everything is healthy; having no endpoints is normal."satisfies the required all-clear, butno endpointsis discarded here because it is followed byis normal, so the factually wrong answer passes. Exculpation needs to distinguish neutral topics such asimage pullfrom polarized claims such asno endpoints(and similarmissing/failurealternatives).
def diagnoses(keyword: str) -> bool:
return _mentions_positively(keyword, answer_tokens, clause_ids, exculpation=True)
ba88532 to
07a3a8a
Compare
A grading rule nobody has run is a guess. Six of the eight bundled journeys had turns whose keyword groups had never been exercised against a specific phrasing, and running them found holes in four journeys that predate this change. The systemic one: a required group naming a *topic* is satisfied by ruling that topic out. "The liveness probe is fine; gateway-1 restarts for another reason" satisfied a group requiring the model to report a liveness probe failure - in logs-to-events, tui-follow, rollout-owner-chain and triage-and-correct. The grader already refused "not X"; declaring X healthy is the same assertion in positive grammar. It now treats a copula plus an all-clear adjective as negation, scoped to the clause. The adjective list is deliberately narrow: "unaffected", "serving" and "ready" are required claims elsewhere in the pack, so admitting them would reject correct answers. The rest cannot be fixed centrally: - triage-and-correct turn 1 required "checkout first" but never forbade the reverse, so an answer could concede it and rank payments above it. - triage-and-correct turn 2 says "focus on payments, not checkout". forbidden_targets stopped the model reading checkout but not blaming it, so naming payments and attributing the cause to checkout-1 passed. - triage-and-correct turn 3 banned two spellings of the wrong remedy; "needs more memory" was neither. - rollout-owner-chain turn 2 asks whether the *old* ReplicaSet still serves. Keyword groups cannot bind a claim to a subject, so "api-7b9d-x1 is still running, while api-5c2f has already been scaled away" passed. The false claim is now banned outright. - namespace-triage turn 2 accepted the bare noun "environment variable". Split into the thing and its absence. test_every_journey_turn_pins_an_accepting_and_a_rejecting_answer makes the exercise mandatory, and counts a turn as covered only when both directions are present - an entry with an empty side pins nothing. Closes #251 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round 1, three findings, all upheld - and the first is a regression this pull request introduced. Exculpation applied to every required keyword, including entity groups. In the `healthy-service-endpoints` negative control, "The service endpoints are healthy" is the correct answer, and suppressing the `endpoints` match because a healthy predicate follows it rejected it. That is the mistake this change exists to prevent, pointed the other way. Exculpation is now off when the scenario has no fault, where the all-clear is the claim being graded rather than a way of dodging it. The other two are the same lesson in journeys: a keyword group binds to nothing outside itself. - namespace-triage split the DSN into a thing group and an absence group, so "DATABASE_DSN is configured; shipper is missing one replica but still serving with 2 of 3" satisfied both while asserting the opposite. Subject and absence now live in one keyword. - triage-and-correct banned the bare subject `checkout`, which rejected "Checkout is not the cause; payments-1 has unauthorized registry credentials" - a negator after the match does not scope back over it. Only misattributing phrasings are banned now. Both reviewed phrasings are pinned. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… polarity Round 2, six suppressed findings, all upheld. Two were structural. Disabling exculpation for a negative control disabled it for the forbidden groups as well, so "Everything is healthy; the image pull looks normal" was rejected as a positive image-pull diagnosis. Polarity is not symmetric: a required all-clear must survive in a scenario with no fault, while a forbidden keyword ruled out with an all-clear predicate is ruled out in every scenario. Split into `claims` and `diagnoses`. `_turn_scenario` hardcoded `root_cause="r"`, so every journey pin was graded as a fault scenario - including `healthy-stop`, which production loads with `root_cause: none`. Now that polarity changes grading, the pins were exercising a configuration that does not exist. It takes `journey.root_cause`. The rest are the same lesson as ever - a list cannot bound a sentence: - `working` left the all-clear set. "The liveness probe is working too slowly and timing out" is a fault claim, and exculpating it dropped the diagnosis being graded. `good` went with it. - triage-and-correct's ordering prohibition omitted the mirror of its own accepted wording, so "checkout should be inspected first, but payments should be inspected first instead" passed. - the same journey enumerated checkout-bound misattributions, which "checkout-1 suffers an authentication failure" walked straight past. The required cause is now bound to payments instead: closing the class rather than listing its members. Article variants are spelled out because matching is contiguous - a closed set, unlike sentence shapes. - namespace-triage rejected "DATABASE_DSN isn't set", because contractions tokenize apart. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round 3, two findings, both upheld and both the same defect: a compound keyword binds two words to each other, not to the workload the turn is asking about. namespace-triage required the DSN absence but not whose DSN, so "Shipper's DATABASE_DSN is missing, but it is still serving; invoicer failed for another reason" satisfied every group while pinning invoicer's fault on the wrong workload. Requiring an invoicer-bound absence would reject the natural answer - "invoicer-1 crashloops because DATABASE_DSN is missing" carries its subject through `because`, not adjacency - so the misattribution is banned instead. triage-and-correct bound payments to the *symptom* as well as the cause, and payments-1 genuinely cannot pull its image, so "payments-1 cannot pull its image because tag v99 does not exist" passed - a diagnosis this fixture contradicts. Symptom-only alternatives are gone; the one phrasing that reaches the cause through the symptom carries the rejection with it. Both phrasings are pinned as rejecting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
07a3a8a to
8fdd66d
Compare
Closes #251.
The finding
I filed this issue expecting to write test scaffolding. Running it found real defects in four journeys that shipped long before the pinning rule — and one of them is systemic.
A required group naming a topic is satisfied by ruling that topic out.
That satisfied
must_mention: [liveness probe, probe failure]— a group whose entire purpose is to require the model to report the probe failure. The same phrasing passedimage pullgroups intui-followandrollout-owner-chain. The grader already refusednot X; declaring X healthy is the same assertion in positive grammar, and nothing caught it.The fix, in two parts
Central. The grader now treats a copula plus an all-clear adjective (
is fine,looks normal,seems correct) as negation, scoped to the clause like the existing negators. The adjective list is deliberately narrow —unaffected,servingandreadyare required claims elsewhere in the pack, so admitting them would reject correct answers. Two tests guard that over-reach:"The image pull failed and the node is fine"must still pass, and so must"api-5c2f is unaffected".Per journey, for the shapes no central rule can catch:
triage-and-correctt1triage-and-correctt2checkout-1—forbidden_targetsstops the read, not the attributioncheckouttriage-and-correctt3rollout-owner-chaint2namespace-triaget2environment variableThe rule
test_every_journey_turn_pins_an_accepting_and_a_rejecting_answerrefuses to let a turn ship without both directions, and counts a turn as covered only when both sides are non-empty — an entry with an empty side pins nothing. Mutation-verified: emptying one side fails the test.18 turns, all pinned. Gate green: 4741 passed.