From 5fda4f7cb1c4245218f698a72da039d647278da8 Mon Sep 17 00:00:00 2001 From: Christopher Maher Date: Thu, 9 Jul 2026 01:24:13 -0700 Subject: [PATCH 1/2] feat(foreman): flag a GO that changes no functional production code A GO whose committed diff is docs/comments/tests only produces a hollow GATE-PASS: the bite check has no production code to revert, so it is vacuous, and prose (docs or Go comments) can claim behavior that is not implemented. On #850 a bug-labeled issue produced a docs + comment-only change whose comment claimed "the operator surfaces a clear warning" with no warning code in the diff, and it GATE-PASSed. Add a non-blocking advisory that runs after repo.Commit (same placement as the grounding rail): if the committed diff (base...HEAD) has no functional production change, record noFunctionalChange=true on the terminal result and log it. functionalChangeInDiff treats docs, tests, and comment-only .go edits as non-functional (diffHasCodeLine is a line heuristic, not a Go parser); degrades open on any git error. Records-and-logs only; never changes the verdict, leaving the call to the reviewer/human. Fixes #1022 Signed-off-by: Christopher Maher --- pkg/foreman/agent/executor_native.go | 7 + .../agent/no_functional_change_gate.go | 115 +++++++++++++++++ .../agent/no_functional_change_gate_test.go | 122 ++++++++++++++++++ 3 files changed, 244 insertions(+) create mode 100644 pkg/foreman/agent/no_functional_change_gate.go create mode 100644 pkg/foreman/agent/no_functional_change_gate_test.go diff --git a/pkg/foreman/agent/executor_native.go b/pkg/foreman/agent/executor_native.go index 54b43eeb..0cc4e126 100644 --- a/pkg/foreman/agent/executor_native.go +++ b/pkg/foreman/agent/executor_native.go @@ -747,6 +747,13 @@ func (e *NativeAgentLoopExecutor) runLLMPath( // never changes the verdict. applyCoderGroundingRailForTask(ctx, log, task, workspace, loopRes) + // No-functional-change advisory (non-blocking): flag a GO whose committed + // diff is docs/comments/tests only, so a "fix" that changes no production + // code (and whose prose may claim unimplemented behavior, #850/#1022) does + // not read as a clean functional GATE-PASS. Same after-commit requirement + // as the grounding rail; records-and-logs, never changes the verdict. + applyNoFunctionalChangeForTask(ctx, log, task, workspace, loopRes) + if err := repo.Push(ctx, repo.PushOptions{ Workspace: workspace, Branch: branch, diff --git a/pkg/foreman/agent/no_functional_change_gate.go b/pkg/foreman/agent/no_functional_change_gate.go new file mode 100644 index 00000000..4cc05533 --- /dev/null +++ b/pkg/foreman/agent/no_functional_change_gate.go @@ -0,0 +1,115 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package agent + +import ( + "context" + "strings" + + "github.com/go-logr/logr" + + foremanv1alpha1 "github.com/defilantech/llmkube/api/foreman/v1alpha1" +) + +// diffHasCodeLine reports whether a unified diff body contains an added or +// removed line that is neither blank nor a comment. It is a best-effort line +// heuristic (not a Go parser): a changed line counts as code unless, after +// dropping the +/- marker and trimming, it is empty or begins with a comment +// token (// or a /* * */ block-comment line). Good enough to tell a +// comments-only edit from a real logic change for an advisory signal. +func diffHasCodeLine(diff string) bool { + for _, l := range strings.Split(diff, "\n") { + if l == "" { + continue + } + if l[0] != '+' && l[0] != '-' { + continue + } + if strings.HasPrefix(l, "+++") || strings.HasPrefix(l, "---") { + continue + } + code := strings.TrimSpace(l[1:]) + switch { + case code == "": + continue + case strings.HasPrefix(code, "//"): + continue + case strings.HasPrefix(code, "/*"), strings.HasPrefix(code, "*"), strings.HasPrefix(code, "*/"): + continue + default: + return true + } + } + return false +} + +// functionalChangeInDiff reports whether the committed diff (base...HEAD) has +// any functional production change: a change to a non-test .go file whose +// added/removed lines are more than comments and blanks. Docs (.md, yaml, +// etc.), tests (_test.go), and comment-only .go edits are non-functional. +// Degrades open (returns true) on any git error so it never spuriously flags. +func functionalChangeInDiff(ctx context.Context, workspace, base string, run commandRunner) bool { + names, err := run(ctx, workspace, nil, "git", "diff", "--name-only", base+"...HEAD") + if err != nil { + return true + } + for _, f := range strings.Split(strings.TrimSpace(names), "\n") { + f = strings.TrimSpace(f) + if f == "" || !strings.HasSuffix(f, ".go") || strings.HasSuffix(f, "_test.go") { + continue + } + out, err := run(ctx, workspace, nil, "git", "diff", "-U0", base+"...HEAD", "--", f) + if err != nil { + return true + } + if diffHasCodeLine(out) { + return true + } + } + return false +} + +// applyNoFunctionalChange records an advisory when a GO's committed diff has +// no functional production change (all changes are docs, comments, or tests). +// This surfaces the #850 class: a "fix" for a code issue that changes no code, +// where prose can claim behavior that is not implemented and the bite check is +// vacuous. Records-and-logs only; never changes the verdict. +func applyNoFunctionalChange(ctx context.Context, log logr.Logger, base, workspace string, loopRes *LoopResult) { + if loopRes == nil || loopRes.Terminal == nil { + return + } + if functionalChangeInDiff(ctx, workspace, base, execCommandRunner) { + return + } + log.Info("coder gate: GO changed no functional production code (docs/comments/tests only)") + if loopRes.Terminal.Extra == nil { + loopRes.Terminal.Extra = map[string]any{} + } + loopRes.Terminal.Extra["noFunctionalChange"] = true +} + +// applyNoFunctionalChangeForTask gates the advisory to issue-fix runs and +// resolves the base branch, mirroring applyCoderGroundingRailForTask. Must run +// after repo.Commit so base...HEAD reflects the committed diff. +func applyNoFunctionalChangeForTask( + ctx context.Context, log logr.Logger, task *foremanv1alpha1.AgenticTask, workspace string, loopRes *LoopResult, +) { + if task.Spec.Kind != foremanv1alpha1.AgenticTaskKindIssueFix { + return + } + applyNoFunctionalChange(ctx, log, baseBranchOrDefault(task.Spec.Payload.BaseBranch), workspace, loopRes) +} diff --git a/pkg/foreman/agent/no_functional_change_gate_test.go b/pkg/foreman/agent/no_functional_change_gate_test.go new file mode 100644 index 00000000..fb9822a4 --- /dev/null +++ b/pkg/foreman/agent/no_functional_change_gate_test.go @@ -0,0 +1,122 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package agent + +import ( + "context" + "errors" + "strings" + "testing" +) + +func TestDiffHasCodeLine(t *testing.T) { + tests := []struct { + name string + diff string + want bool + }{ + {"comment-only additions", "@@ -1,0 +2,2 @@\n+// a new comment\n+// more comment\n", false}, + {"blank additions", "@@ -1,0 +2,1 @@\n+ \n", false}, + {"block comment lines", "@@ -1,0 +2,3 @@\n+/* doc\n+ * lines\n+ */\n", false}, + {"real code addition", "@@ -1,0 +2,1 @@\n+x := doWork()\n", true}, + {"real code removal", "@@ -2,1 +1,0 @@\n-return err\n", true}, + {"headers ignored, only comment body", "--- a/x.go\n+++ b/x.go\n@@ -1 +1 @@\n+// tweak\n", false}, + {"code among comments", "@@ -1,0 +2,2 @@\n+// note\n+cfg.Enabled = true\n", true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := diffHasCodeLine(tc.diff); got != tc.want { + t.Errorf("diffHasCodeLine = %v, want %v", got, tc.want) + } + }) + } +} + +func TestFunctionalChangeInDiff(t *testing.T) { + // runner dispatches on the git subcommand: --name-only returns the file + // list; a per-file `diff -U0 ... -- ` returns that file's diff body. + runner := func(names string, perFile map[string]string, nameErr error) commandRunner { + return func(_ context.Context, _ string, _ []string, _ string, args ...string) (string, error) { + if argsContain(args, "--name-only") { + return names, nameErr + } + // last arg is the file path (after the "--" separator). + f := args[len(args)-1] + return perFile[f], nil + } + } + + tests := []struct { + name string + names string + perFile map[string]string + nameErr error + want bool + }{ + { + name: "docs only", + names: "docs/model-storage.md\nREADME.md", + want: false, + }, + { + name: "tests only", + names: "pkg/foreman/agent/thing_test.go", + want: false, + }, + { + name: "comment-only production go", + names: "internal/controller/model_storage.go", + perFile: map[string]string{"internal/controller/model_storage.go": "@@ -55,0 +56,3 @@\n+// On a tainted node...\n+// pre-stage the GGUF...\n"}, + want: false, + }, + { + name: "real production go change", + names: "internal/controller/model_storage.go", + perFile: map[string]string{"internal/controller/model_storage.go": "@@ -55,0 +56,1 @@\n+recorder.Eventf(obj, \"Warning\", \"Tainted\", msg)\n"}, + want: true, + }, + { + name: "docs plus comment-only go", + names: "docs/x.md\ninternal/controller/model_storage.go", + perFile: map[string]string{"internal/controller/model_storage.go": "@@ -1,0 +2,1 @@\n+// just a comment\n"}, + want: false, + }, + { + name: "git error degrades open (assume functional)", + names: "", + nameErr: errors.New("boom"), + want: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + run := runner(tc.names, tc.perFile, tc.nameErr) + if got := functionalChangeInDiff(context.Background(), "/ws", "main", run); got != tc.want { + t.Errorf("functionalChangeInDiff = %v, want %v", got, tc.want) + } + }) + } +} + +func argsContain(ss []string, want string) bool { + for _, s := range ss { + if strings.Contains(s, want) { + return true + } + } + return false +} From 24420bb76f1315ccc45830e28a071e9d70eb147d Mon Sep 17 00:00:00 2001 From: Christopher Maher Date: Thu, 9 Jul 2026 08:57:13 -0700 Subject: [PATCH 2/2] test(foreman): wrap long lines in no-functional-change test (lll) Signed-off-by: Christopher Maher --- .../agent/no_functional_change_gate_test.go | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/pkg/foreman/agent/no_functional_change_gate_test.go b/pkg/foreman/agent/no_functional_change_gate_test.go index fb9822a4..941f34e9 100644 --- a/pkg/foreman/agent/no_functional_change_gate_test.go +++ b/pkg/foreman/agent/no_functional_change_gate_test.go @@ -78,16 +78,20 @@ func TestFunctionalChangeInDiff(t *testing.T) { want: false, }, { - name: "comment-only production go", - names: "internal/controller/model_storage.go", - perFile: map[string]string{"internal/controller/model_storage.go": "@@ -55,0 +56,3 @@\n+// On a tainted node...\n+// pre-stage the GGUF...\n"}, - want: false, + name: "comment-only production go", + names: "internal/controller/model_storage.go", + perFile: map[string]string{ + "internal/controller/model_storage.go": "@@ -55,0 +56,1 @@\n+// a comment\n", + }, + want: false, }, { - name: "real production go change", - names: "internal/controller/model_storage.go", - perFile: map[string]string{"internal/controller/model_storage.go": "@@ -55,0 +56,1 @@\n+recorder.Eventf(obj, \"Warning\", \"Tainted\", msg)\n"}, - want: true, + name: "real production go change", + names: "internal/controller/model_storage.go", + perFile: map[string]string{ + "internal/controller/model_storage.go": "@@ -55,0 +56,1 @@\n+recorder.Eventf(o, m)\n", + }, + want: true, }, { name: "docs plus comment-only go",