From 8d60ba66afbcb9e8eb68837e3fa52c8645d3b739 Mon Sep 17 00:00:00 2001 From: Christopher Maher Date: Thu, 9 Jul 2026 00:59:25 -0700 Subject: [PATCH] feat: semantic issueAsk verification instead of verbatim substring The reviewer's issueAsk rail previously required the model's stated ask to be a literal substring of the fetched issue body. This conflated "paraphrased but correct" with "wrong issue entirely": both produced `issueAskVerified: false` and demoted GO to NO-GO, including cases where the reviewer understood the scope faithfully. Replace the verbatim check with a two-tier gate: 1. Verbatim substring match -> verified immediately (unchanged). 2. Keyword-overlap check between claim and issue body -> verified if the claim references a sufficient fraction of the issue's salient nouns/verbs (threshold: 40%, minimum 2 keywords). 3. Neither passes -> archive claim, rewrite from body, mark unverified (unchanged). The keyword extractor strips markdown, lowercases, removes common English stop words, and filters tokens <= 2 chars. It takes the title plus the first two paragraphs of the body. This is more explainable than raw embedding similarity and requires no external model. Surface the method used (`issueAskMethod: semantic` or `rewritten`) next to `issueAskVerified` so demotions are auditable. Update the demotion reason strings and docs to reflect the new semantics. Fixes #809 Signed-off-by: Christopher Maher --- docs/site/foreman/model-compatibility.md | 13 +- pkg/foreman/agent/executor_native.go | 151 ++++++++++++++++-- .../agent/executor_native_internal_test.go | 89 +++++++++++ 3 files changed, 238 insertions(+), 15 deletions(-) diff --git a/docs/site/foreman/model-compatibility.md b/docs/site/foreman/model-compatibility.md index 188cc08c..96b29e80 100644 --- a/docs/site/foreman/model-compatibility.md +++ b/docs/site/foreman/model-compatibility.md @@ -54,11 +54,14 @@ before the result is stored: Shipped in [#584](https://github.com/defilantech/LLMKube/pull/584). - **`issueAsk`** is checked against the body the model fetched via - the `fetch_issue` tool. If the claim is a literal substring of - the body it's marked verified; otherwise it's archived at - `issueAskClaimed` and rewritten with the first useful paragraph - of the body. Shipped in - [#587](https://github.com/defilantech/LLMKube/pull/587). + the `fetch_issue` tool. A verbatim substring is marked verified + immediately; otherwise a keyword-overlap check decides whether the + claim semantically covers the issue (faithful paraphrase) or is a + hallucination. Verified claims land at `issueAskMethod=semantic`; + unverified claims are archived at `issueAskClaimed` and rewritten + with the first useful paragraph of the body. Shipped in + [#587](https://github.com/defilantech/LLMKube/pull/587), enhanced + in [#809](https://github.com/defilantech/LLMKube/issues/809). A new boolean field `issueAskVerified` signals to downstream consumers whether the stored value came from the model diff --git a/pkg/foreman/agent/executor_native.go b/pkg/foreman/agent/executor_native.go index 54b43eeb..2216885d 100644 --- a/pkg/foreman/agent/executor_native.go +++ b/pkg/foreman/agent/executor_native.go @@ -1923,11 +1923,13 @@ func fileListsEqual(prev any, groundTruth []string) bool { // reconcileReviewerIssueAsk grounds the reviewer's // `submit_result.extra.issueAsk` field against the real issue body // that came back from the reviewer's `fetch_issue` tool call. The -// claim has to be a literal substring of that body; if it is not, -// the harness archives the model's claim under `issueAskClaimed` -// and rewrites `issueAsk` with the first useful prose paragraph of -// the body (skipping markdown headers). Pairs with #582's filesTouched -// fix: same shape, different field. +// claim must semantically cover the issue: a verbatim substring is +// accepted immediately, otherwise a key-requirement overlap check +// decides whether the reviewer paraphrased correctly or confabulated. +// The harness archives the model's claim under `issueAskClaimed` and +// rewrites `issueAsk` with the first useful prose paragraph of the +// body (skipping markdown headers) when the claim is ungrounded. +// Pairs with #582's filesTouched fix: same shape, different field. // // Why this is structural rather than a prompt-tightening fix: the // post-#584 rereview showed devstral on the Mac Studio confabulating @@ -1969,19 +1971,148 @@ func reconcileReviewerIssueAsk(log logr.Logger, msgs []oai.Message, extra map[st extra["issueAskVerified"] = true return } + // Verbatim miss: check whether the claim semantically covers the + // issue by requiring sufficient overlap with the issue's key + // nouns/requirements. A faithful paraphrase passes; a hallucination + // does not. + if issueAskSemanticallyCovers(claim, body) { + extra["issueAskVerified"] = true + extra["issueAskMethod"] = "semantic" + log.Info("reviewer issueAsk: claim verified via semantic coverage", + "modelClaim", claim) + return + } // Confabulation: claim is not a substring of the body the model - // itself fetched. Archive it and rewrite issueAsk with a real - // excerpt from the body. + // itself fetched, and does not semantically cover it either. Archive + // it and rewrite issueAsk with a real excerpt from the body. extra["issueAskClaimed"] = claim replaced := firstBodyParagraph(body, 200) extra["issueAsk"] = replaced extra["issueAskVerified"] = false + extra["issueAskMethod"] = "rewritten" log.Info("reviewer issueAsk: model claim not a substring of fetch_issue body; rewriting from body", "modelClaim", claim, "rewrittenTo", replaced, ) } +// issueAskSemanticallyCovers reports whether the reviewer's stated ask +// (claim) semantically covers the fetched issue body. The check is +// keyword-overlap based: extract the issue's salient nouns/requirements +// (nouns and verbs from the title + first two paragraphs) and require +// the claim to reference a sufficient fraction of them. This is more +// explainable than raw similarity and does not require an embedding +// model. +// +// The verbatim path is handled by the caller; this function is only +// invoked when the claim is not a substring of the body. +func issueAskSemanticallyCovers(claim, body string) bool { + keywords := extractIssueKeywords(body) + if len(keywords) == 0 { + return false + } + claimLower := strings.ToLower(claim) + covered := 0 + for _, kw := range keywords { + if strings.Contains(claimLower, kw) { + covered++ + } + } + // Require at least 40% coverage, minimum 2 keywords, to avoid + // false positives on very short claims. + threshold := len(keywords) * 2 / 5 + if threshold < 2 { + threshold = 2 + } + return covered >= threshold +} + +// extractIssueKeywords pulls salient nouns and verbs from the issue +// title and first two paragraphs. It lowercases, strips punctuation, +// removes common English stop words, and returns unique tokens longer +// than 2 characters. +func extractIssueKeywords(body string) []string { + stopWords := map[string]bool{ + "a": true, "an": true, "the": true, "and": true, "or": true, + "but": true, "in": true, "on": true, "at": true, "to": true, + "for": true, "of": true, "with": true, "by": true, "from": true, + "is": true, "are": true, "was": true, "were": true, "be": true, + "been": true, "being": true, "have": true, "has": true, "had": true, + "do": true, "does": true, "did": true, "will": true, "would": true, + "could": true, "should": true, "may": true, "might": true, + "can": true, "shall": true, "it": true, "its": true, "this": true, + "that": true, "these": true, "those": true, "i": true, "we": true, + "they": true, "he": true, "she": true, "you": true, "me": true, + "my": true, "your": true, "his": true, "her": true, "our": true, + "their": true, "what": true, "which": true, "who": true, "whom": true, + "where": true, "when": true, "why": true, "how": true, "not": true, + "no": true, "nor": true, "so": true, "yet": true, "both": true, + "each": true, "few": true, "more": true, "most": true, "other": true, + "some": true, "such": true, "than": true, "too": true, "very": true, + "just": true, "also": true, "now": true, "here": true, "there": true, + "then": true, "once": true, "if": true, "as": true, "into": true, + "about": true, "up": true, "out": true, "off": true, "over": true, + "under": true, "again": true, "further": true, "all": true, "any": true, + } + // Take title + first two paragraphs. + lines := strings.Split(body, "\n") + var textParts []string + for _, l := range lines { + stripped := strings.TrimSpace(l) + if stripped == "" { + continue + } + if strings.HasPrefix(stripped, "#") { + textParts = append(textParts, stripped) + continue + } + textParts = append(textParts, stripped) + if len(textParts) >= 3 { + break + } + } + text := strings.Join(textParts, " ") + // Strip markdown formatting. + text = strings.ReplaceAll(text, "**", "") + text = strings.ReplaceAll(text, "*", "") + text = strings.ReplaceAll(text, "`", "") + text = strings.ReplaceAll(text, "_", "") + // Lowercase and split on non-alphanumeric. + text = strings.ToLower(text) + var tokens []string + var current strings.Builder + for _, r := range text { + if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' { + current.WriteRune(r) + } else { + if current.Len() > 0 { + tokens = append(tokens, current.String()) + current.Reset() + } + } + } + if current.Len() > 0 { + tokens = append(tokens, current.String()) + } + // Filter: length > 2, not a stop word, unique. + seen := map[string]bool{} + var keywords []string + for _, t := range tokens { + if len(t) <= 2 { + continue + } + if stopWords[t] { + continue + } + if seen[t] { + continue + } + seen[t] = true + keywords = append(keywords, t) + } + return keywords +} + // enforceReviewerIssueAsk converts a failed issueAsk verification from // an observation into a routing decision (#644). reconcileReviewerIssueAsk // records whether the model's stated understanding of the issue is a @@ -2030,7 +2161,7 @@ func enforceReviewerIssueAsk( extra["verdictClaimed"] = string(verdict) if verdict != foremanv1alpha1.AgenticTaskVerdictGo { - extra["demotionReason"] = "issueAsk could not be verified as a verbatim quote of the " + + extra["demotionReason"] = "issueAsk could not be verified as covering the " + "fetched issue body; review verdict is untrusted" log.Info("reviewer integrity: unverified issueAsk on non-GO verdict; keeping verdict but marking untrusted", "verdict", verdict) @@ -2045,14 +2176,14 @@ func enforceReviewerIssueAsk( if scopeVouches { extra["issueAskVerified"] = false extra["scopeVouched"] = true - extra["demotionReason"] = "issueAsk could not be verified as a verbatim quote of the " + + extra["demotionReason"] = "issueAsk could not be verified as covering the " + "fetched issue body; scope-overlap confirms in-scope review" log.Info("reviewer integrity: unverified issueAsk on GO verdict; scope-overlap vouches, keeping GO", "scopeMatched", scopeMatched) return verdict } - extra["demotionReason"] = "issueAsk could not be verified as a verbatim quote of the " + + extra["demotionReason"] = "issueAsk could not be verified as covering the " + "fetched issue body; review verdict is untrusted" log.Info("reviewer integrity: unverified issueAsk on GO verdict; demoting to NO-GO", "verdictClaimed", verdict) diff --git a/pkg/foreman/agent/executor_native_internal_test.go b/pkg/foreman/agent/executor_native_internal_test.go index b38699ee..c0e17382 100644 --- a/pkg/foreman/agent/executor_native_internal_test.go +++ b/pkg/foreman/agent/executor_native_internal_test.go @@ -1370,6 +1370,95 @@ func TestReconcileReviewerIssueAsk_NilExtraIsNoOp(t *testing.T) { reconcileReviewerIssueAsk(logr.Discard(), nil, nil) // must not panic } +func TestReconcileReviewerIssueAsk_ParaphraseVerified(t *testing.T) { + body := "## Feature Description\n\nUpdate the toolchain image to v2 in the agent-builder Dockerfile so golangci-lint and controller-gen are available at build time." + content, _ := json.Marshal(map[string]string{"body": body}) + msgs := []oai.Message{ + {Role: oai.RoleAssistant, ToolCalls: []oai.ToolCall{{ + ID: "tc-1", Type: "function", + Function: oai.ToolCallFunction{Name: "fetch_issue"}, + }}}, + {Role: oai.RoleTool, ToolCallID: "tc-1", Content: string(content)}, + } + // Faithful paraphrase: mentions the key nouns (toolchain, image, v2, agent-builder, golangci-lint, controller-gen). + claim := "Update the toolchain image to v2 in the agent-builder so golangci-lint and controller-gen are available." + extra := map[string]any{"issueAsk": claim} + reconcileReviewerIssueAsk(logr.Discard(), msgs, extra) + if v, _ := extra["issueAskVerified"].(bool); !v { + t.Errorf("faithful paraphrase should be verified via semantic coverage; got %v", extra["issueAskVerified"]) + } + if extra["issueAsk"] != claim { + t.Errorf("paraphrase should be preserved unchanged; got %v", extra["issueAsk"]) + } + if extra["issueAskMethod"] != "semantic" { + t.Errorf("semantic verification should set issueAskMethod=semantic; got %v", extra["issueAskMethod"]) + } +} + +func TestReconcileReviewerIssueAsk_HallucinationRewritten(t *testing.T) { + body := "## Feature Description\n\nUpdate the toolchain image to v2 in the agent-builder Dockerfile so golangci-lint and controller-gen are available at build time." + content, _ := json.Marshal(map[string]string{"body": body}) + msgs := []oai.Message{ + {Role: oai.RoleAssistant, ToolCalls: []oai.ToolCall{{ + ID: "tc-1", Type: "function", + Function: oai.ToolCallFunction{Name: "fetch_issue"}, + }}}, + {Role: oai.RoleTool, ToolCallID: "tc-1", Content: string(content)}, + } + // Confabulation: mentions unrelated nouns (cache, list, CLI). + claim := "enhance `llmkube cache list` to show cached model digests." + extra := map[string]any{"issueAsk": claim} + reconcileReviewerIssueAsk(logr.Discard(), msgs, extra) + if v, _ := extra["issueAskVerified"].(bool); v { + t.Errorf("hallucinated claim should not be verified; got %v", extra["issueAskVerified"]) + } + if extra["issueAsk"] == claim { + t.Errorf("hallucinated claim should be rewritten; got %v", extra["issueAsk"]) + } + if extra["issueAskClaimed"] != claim { + t.Errorf("issueAskClaimed should preserve original claim; got %v", extra["issueAskClaimed"]) + } +} + +func TestExtractIssueKeywords_Basic(t *testing.T) { + body := "## Feature Description\n\nUpdate the toolchain image to v2 in the agent-builder Dockerfile so golangci-lint and controller-gen are available at build time." + kw := extractIssueKeywords(body) + if len(kw) == 0 { + t.Fatalf("expected non-empty keywords") + } + // Should include salient nouns. + found := map[string]bool{} + for _, k := range kw { + found[k] = true + } + for _, want := range []string{"toolchain", "image", "agent-builder", "golangci-lint", "controller-gen"} { + if !found[want] { + t.Errorf("expected keyword %q in %v", want, kw) + } + } +} + +func TestExtractIssueKeywords_StopsFiltered(t *testing.T) { + body := "the a an the feature is implemented with the new tool" + kw := extractIssueKeywords(body) + for _, k := range kw { + if k == "the" || k == "a" || k == "an" || k == "is" || k == "with" { + t.Errorf("stop word %q should be filtered; got %v", k, kw) + } + } +} + +func TestIssueAskSemanticallyCovers_ShortClaim(t *testing.T) { + // Short claim with enough keyword coverage should pass. + body := "## Feature\n\nUpdate the toolchain image to v2 in the agent-builder Dockerfile so golangci-lint and controller-gen are available at build time." + if !issueAskSemanticallyCovers("update toolchain image agent-builder golangci-lint controller-gen", body) { + t.Errorf("claim covering enough keywords should pass") + } + if issueAskSemanticallyCovers("xyz abc def ghi jkl", body) { + t.Errorf("irrelevant claim should fail") + } +} + func TestEnforceReviewerIssueAsk_VerifiedGoStands(t *testing.T) { extra := map[string]any{"issueAskVerified": true} got := enforceReviewerIssueAsk(logr.Discard(), extra, foremanv1alpha1.AgenticTaskVerdictGo, false, nil)