Conversation
15 test cases across 3 evals targeting failure-prone behaviors: - fix-cve (6 cases, 8 judges): replace directive syntax, multi-module detection, command ordering, path C hard stop, negative case, commit format - go-lint (5 cases, 5 judges): discovery cascade, read-only constraint, not-installed handling - go-lint-fix (4 cases, 6 judges): generated file skip, real fix over nolint, existing constant reuse, direct invocation with --fix eval-fix-cve validated: 7/7 deterministic judges at 100% pass rate on claude-sonnet-4-20250514 ($0.82, 292s for 6 cases). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: miyadav The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds three Go plugin evaluation suites for CVE fixes, lint discovery, and lint fixing, with case fixtures, annotations, structured outputs, automated judges, quality rubrics, execution settings, usage documentation, and version updates. ChangesGo plugin evaluation suites
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EvalHarness
participant GoSkill
participant Judges
EvalHarness->>GoSkill: provide evaluation case
GoSkill->>EvalHarness: write structured plan JSON
EvalHarness->>Judges: submit plan and annotations
Judges->>EvalHarness: return constraint and quality scores
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)plugins/golang/evals/eval-fix-cve.yamlTraceback (most recent call last): plugins/golang/evals/eval-go-lint-fix.yamlTraceback (most recent call last): plugins/golang/evals/eval-go-lint.yamlTraceback (most recent call last): Comment |
CI requires version bump for any plugin file changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: 12
🤖 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
`@plugins/golang/evals/cases/fix-cve/case-001-replace-directive-syntax/input.yaml`:
- Around line 7-11: Update the fix-cve fixture input to satisfy the Path C
hard-stop rule by adding the required fork_url alongside fork_module and
fork_version, or revise the rule to explicitly allow module path/version as the
replace source. Preserve the expected replace-directive behavior without
permitting a compliant implementation to stop due to missing fork URL.
In
`@plugins/golang/evals/cases/fix-cve/case-003-command-ordering/annotations.yaml`:
- Around line 1-13: Update the CVE-fix judge logic for
plugins/golang/evals/cases/fix-cve/case-003-command-ordering/annotations.yaml
lines 1-13 to enforce make update before make verify and require GOWORK=off and
GOFLAGS="" specifically on tidy/vendor commands. Also update
plugins/golang/evals/cases/fix-cve/case-005-module-not-affected/annotations.yaml
lines 1-2 so the judge fails when files or vendor state change, even if
not_affected is true.
In `@plugins/golang/evals/eval-fix-cve.yaml`:
- Around line 77-100: The fix-plan instructions conflict by requiring only a
commit message for commit-message cases while also requiring the full JSON
output. Update the “For commit_message cases” rule to keep the required
fix-plan.json structure and strategy fields, placing the commit message only in
the commit_message property.
- Around line 268-272: The validation after the hard_stop check must require a
non-empty hard_stop_reason that explicitly asks the user for the missing fork
information. Update the logic around the hard_stop validation to inspect that
specific field rather than accepting unrelated occurrences of “ask”, “stop”,
“user”, or “fork” elsewhere in the serialized output.
- Around line 309-320: The annotation validation block must enforce both
expected_action_word and the expected ticket value instead of only checking for
any OCPBUGS- prefix. Update the subject validation to compare its action word
with the annotation, and update the expected_ticket_on_own_line path to compare
a standalone ticket line against the provided annotation/input ticket value
while preserving the existing error reporting pattern.
- Around line 153-159: Update the validation logic around the strategy result to
require and compare the reported strategy against annotations.expected_strategy,
and require the affected-modules field when
annotations.expected_affected_modules is present. Validate that the reported
modules match the expected modules, while preserving the existing A/B/C strategy
validation and error-reporting behavior.
- Around line 34-38: Update the Path A and Path B criteria in the Go CVE
evaluation guidance so they cannot overlap: define Path A only for projects with
a strictly higher Go minor than the required fix, while reserving same-minor
lower-patch cases exclusively for Path B. Keep the existing examples and Path C
behavior consistent with these mutually exclusive conditions.
- Around line 175-195: Update the replace-directive validation block to parse
the directive structurally as “replace <original> => <fork> <version>” rather
than checking substrings. Compare the parsed original module, fork module, and
fork version against the fixture expectations, including
expected_fork_version_in_replace, while preserving the existing CVE-comment
validation and clear errors for malformed or mismatched directives.
- Around line 245-252: Update the validation logic around the expected_vars
check to inspect each command entry individually rather than searching
serialized full_text. Locate the go mod tidy and go mod vendor commands in the
plan, validate each command’s env field contains the required environment
variables, and report missing variables per command while preserving the
existing success/failure return contract.
In `@plugins/golang/evals/eval-go-lint-fix.yaml`:
- Around line 163-184: Update the existing_constant_reuse check so it requires
at least one files_to_modify entry with reuses_existing_constant set to true,
rather than failing when any entry is false. Preserve the existing
DefaultTimeout reference check and the not-applicable behavior, allowing
legitimate new constants such as MaxRetries in the same file.
In `@plugins/golang/evals/eval-go-lint.yaml`:
- Around line 97-187: The deterministic judges must enforce all declared
annotations and read-only behavior. Update valid_output_json to require
discovery_steps, extend discovery_command_correct to validate
expected_discovery_source and both cascade-order flags, and add checks for
expected_reports_issues, expected_suggests_lint_fix, and expected_does_not_fix.
Strengthen read_only_constraint to reject any modified_files entry other than
lint-plan.json, and scan the complete serialized plan for auto-install
directives in not_installed_handling.
In `@plugins/golang/evals/README.md`:
- Around line 81-85: Update the fixture-tree fenced code block in the README to
specify the text language identifier, preserving the existing directory layout
content so Markdownlint MD040 passes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bf1b7617-3695-476b-b9b9-9604f5227133
📒 Files selected for processing (34)
plugins/golang/evals/README.mdplugins/golang/evals/cases/fix-cve/case-001-replace-directive-syntax/annotations.yamlplugins/golang/evals/cases/fix-cve/case-001-replace-directive-syntax/input.yamlplugins/golang/evals/cases/fix-cve/case-002-multi-module-detection/annotations.yamlplugins/golang/evals/cases/fix-cve/case-002-multi-module-detection/input.yamlplugins/golang/evals/cases/fix-cve/case-003-command-ordering/annotations.yamlplugins/golang/evals/cases/fix-cve/case-003-command-ordering/input.yamlplugins/golang/evals/cases/fix-cve/case-004-path-c-hard-stop/annotations.yamlplugins/golang/evals/cases/fix-cve/case-004-path-c-hard-stop/input.yamlplugins/golang/evals/cases/fix-cve/case-005-module-not-affected/annotations.yamlplugins/golang/evals/cases/fix-cve/case-005-module-not-affected/input.yamlplugins/golang/evals/cases/fix-cve/case-006-commit-message-format/annotations.yamlplugins/golang/evals/cases/fix-cve/case-006-commit-message-format/input.yamlplugins/golang/evals/cases/go-lint-fix/case-001-generated-file-skip/annotations.yamlplugins/golang/evals/cases/go-lint-fix/case-001-generated-file-skip/input.yamlplugins/golang/evals/cases/go-lint-fix/case-002-real-fix-not-nolint/annotations.yamlplugins/golang/evals/cases/go-lint-fix/case-002-real-fix-not-nolint/input.yamlplugins/golang/evals/cases/go-lint-fix/case-003-existing-constant-reuse/annotations.yamlplugins/golang/evals/cases/go-lint-fix/case-003-existing-constant-reuse/input.yamlplugins/golang/evals/cases/go-lint-fix/case-004-direct-invocation-not-make/annotations.yamlplugins/golang/evals/cases/go-lint-fix/case-004-direct-invocation-not-make/input.yamlplugins/golang/evals/cases/go-lint/case-001-discovery-claudemd/annotations.yamlplugins/golang/evals/cases/go-lint/case-001-discovery-claudemd/input.yamlplugins/golang/evals/cases/go-lint/case-002-discovery-makefile/annotations.yamlplugins/golang/evals/cases/go-lint/case-002-discovery-makefile/input.yamlplugins/golang/evals/cases/go-lint/case-003-discovery-fallback/annotations.yamlplugins/golang/evals/cases/go-lint/case-003-discovery-fallback/input.yamlplugins/golang/evals/cases/go-lint/case-004-read-only-constraint/annotations.yamlplugins/golang/evals/cases/go-lint/case-004-read-only-constraint/input.yamlplugins/golang/evals/cases/go-lint/case-005-not-installed-handling/annotations.yamlplugins/golang/evals/cases/go-lint/case-005-not-installed-handling/input.yamlplugins/golang/evals/eval-fix-cve.yamlplugins/golang/evals/eval-go-lint-fix.yamlplugins/golang/evals/eval-go-lint.yaml
Comment on lines
+7
to
+11
| fork_module: "github.com/openshift-sustaining/grpc-go" | ||
| fork_version: "v1.71.3-sec.1" | ||
| fork_info: | | ||
| Fork module: github.com/openshift-sustaining/grpc-go | ||
| Fork version: v1.71.3-sec.1 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the fork input with the Path C hard-stop rule.
This fixture provides a module path and version, but no fork URL. Since the workflow says Path C must stop when no fork URL is provided, a compliant model can hard-stop instead of producing the expected replace directive. Either provide fork_url or define the rule in terms of the module path/version actually needed for replace.
🤖 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
`@plugins/golang/evals/cases/fix-cve/case-001-replace-directive-syntax/input.yaml`
around lines 7 - 11, Update the fix-cve fixture input to satisfy the Path C
hard-stop rule by adding the required fork_url alongside fork_module and
fork_version, or revise the rule to explicitly allow module path/version as the
replace source. Preserve the expected replace-directive behavior without
permitting a compliant implementation to stop due to missing fork URL.
Contributor
Author
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the fork input with the Path C hard-stop rule.
This fixture provides a module path and version, but no fork URL. Since the workflow says Path C must stop when no fork URL is provided, a compliant model can hard-stop instead of producing the expected replace directive. Either provide
fork_urlor define the rule in terms of the module path/version actually needed forreplace.🤖 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 `@plugins/golang/evals/cases/fix-cve/case-001-replace-directive-syntax/input.yaml` around lines 7 - 11, Update the fix-cve fixture input to satisfy the Path C hard-stop rule by adding the required fork_url alongside fork_module and fork_version, or revise the rule to explicitly allow module path/version as the replace source. Preserve the expected replace-directive behavior without permitting a compliant implementation to stop due to missing fork URL.
fork url is provided as fork_info
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fixture ---'
sed -n '1,80p' plugins/golang/evals/cases/fix-cve/case-001-replace-directive-syntax/input.yaml
printf '%s\n' '--- Path C / fork-related evaluator references ---'
rg -n -i -C 4 'fork_info|fork_url|Path C|hard.stop|replace directive' \
plugins/golang/evals/eval-fix-cve.yaml \
plugins/golang/evals/cases/fix-cve/case-001-replace-directive-syntax \
plugins/golang/evals/cases/fix-cve/case-004-path-c-hard-stopLength of output: 16689
@miyadav, I verified the current fixture and evaluator. fork_info provides the fork module path and version, but not a URL:
fork_info: |
Fork module: github.com/openshift-sustaining/grpc-go
Fork version: v1.71.3-sec.1Meanwhile, plugins/golang/evals/eval-fix-cve.yaml explicitly says Path C must hard-stop if “no fork URL was provided” (Lines 48 and 97), without defining fork_info as an equivalent. Therefore, the ambiguity remains: a literal compliant implementation may stop rather than generate the expected directive.
Please either add an explicit fork_url (for example, the upstream fork repository URL) or revise the hard-stop rule to require missing fork module/version information instead.
Comment on lines
+1
to
+13
| expected_command_order: | ||
| - phase: "vendor_sync" | ||
| commands: ["go mod tidy", "go mod vendor"] | ||
| must_precede: "make_update" | ||
| - phase: "make_update" | ||
| commands: ["make update"] | ||
| must_precede: "make_verify" | ||
| - phase: "make_verify" | ||
| commands: ["make verify"] | ||
| expected_env_vars: | ||
| - "GOWORK=off" | ||
| - "GOFLAGS=" | ||
| expected_vendor_before_make: true |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ensure the CVE-fix judges fully enforce the declared annotation contracts.
The shown workflow checks only a subset of the expectations, allowing invalid plans to pass.
plugins/golang/evals/cases/fix-cve/case-003-command-ordering/annotations.yaml#L1-L13: enforcemake updatebeforemake verify, and requireGOWORK=off/GOFLAGS=""specifically on tidy/vendor commands.plugins/golang/evals/cases/fix-cve/case-005-module-not-affected/annotations.yaml#L1-L2: fail when files or vendor state change, not only whennot_affectedis false.
📍 Affects 2 files
plugins/golang/evals/cases/fix-cve/case-003-command-ordering/annotations.yaml#L1-L13(this comment)plugins/golang/evals/cases/fix-cve/case-005-module-not-affected/annotations.yaml#L1-L2
🤖 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
`@plugins/golang/evals/cases/fix-cve/case-003-command-ordering/annotations.yaml`
around lines 1 - 13, Update the CVE-fix judge logic for
plugins/golang/evals/cases/fix-cve/case-003-command-ordering/annotations.yaml
lines 1-13 to enforce make update before make verify and require GOWORK=off and
GOFLAGS="" specifically on tidy/vendor commands. Also update
plugins/golang/evals/cases/fix-cve/case-005-module-not-affected/annotations.yaml
lines 1-2 so the judge fails when files or vendor state change, even if
not_affected is true.
Comment on lines
+34
to
+38
| - **Path A** (Direct update): Project Go >= Fix Go (same or higher minor). | ||
| Example: project go 1.25.0, fix needs go 1.23.1 → direct update works. | ||
| - **Path B** (Bump Go patch): Same minor, lower patch. | ||
| Example: project go 1.23.1, fix needs go 1.23.7 → bump patch, then update. | ||
| - **Path C** (Fork replace): Project Go minor < Fix Go minor. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make Paths A and B mutually exclusive.
Path A includes the “same minor” case, while Path B also handles the same minor with a lower patch. For example, 1.23.1 versus 1.23.7 matches both rules. Define Path A as a strictly higher minor, or explicitly exclude lower patches.
🤖 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 `@plugins/golang/evals/eval-fix-cve.yaml` around lines 34 - 38, Update the Path
A and Path B criteria in the Go CVE evaluation guidance so they cannot overlap:
define Path A only for projects with a strictly higher Go minor than the
required fix, while reserving same-minor lower-patch cases exclusively for Path
B. Keep the existing examples and Path C behavior consistent with these mutually
exclusive conditions.
Comment on lines
+163
to
+184
| - name: existing_constant_reuse | ||
| description: Verify the skill reuses existing constants instead of creating duplicates | ||
| check: | | ||
| import json | ||
| ann = outputs.get("annotations", {}) | ||
| if not ann.get("expected_uses_existing_constant"): | ||
| return (True, "Not a constant-reuse case") | ||
| files = outputs.get("files", {}) | ||
| modified = outputs.get("modified_files", {}) | ||
| all_f = {**files, **modified} | ||
| result_files = {k: v for k, v in all_f.items() if k.endswith("lint-fix-plan.json")} | ||
| if not result_files: | ||
| return (False, "No lint-fix-plan.json found") | ||
| data = json.loads(list(result_files.values())[0]) | ||
| full_text = json.dumps(data).lower() | ||
| if "defaulttimeout" not in full_text: | ||
| return (False, "Should reference existing constant DefaultTimeout") | ||
| to_modify = data.get("files_to_modify", []) | ||
| for f in to_modify: | ||
| if f.get("reuses_existing_constant") is False: | ||
| return (False, "Should reuse existing constant, not create a new one") | ||
| return (True, "Correctly reuses existing constant") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
existing_constant_reuse judge can fail a legitimately-correct plan.
The check fails the whole case if any files_to_modify entry has reuses_existing_constant: false, but per case-003's own annotation notes, a new constant for MaxRetries (semantically distinct from DefaultTimeout) "may be appropriate there." Since files_to_modify entries are per-file (not per-issue), a model that correctly reuses DefaultTimeout for Timeout but creates a new constant for MaxRetries on the same server.go entry would be marked as violating the constraint — even though that's the documented acceptable behavior. The lower min_pass_rate: 0.85 threshold (vs. 1.0 elsewhere) suggests this flakiness was already observed rather than root-caused.
🔧 Suggested fix: require at least one reuse, don't fail on any non-reuse
data = json.loads(list(result_files.values())[0])
full_text = json.dumps(data).lower()
if "defaulttimeout" not in full_text:
return (False, "Should reference existing constant DefaultTimeout")
to_modify = data.get("files_to_modify", [])
- for f in to_modify:
- if f.get("reuses_existing_constant") is False:
- return (False, "Should reuse existing constant, not create a new one")
- return (True, "Correctly reuses existing constant")
+ if not any(f.get("reuses_existing_constant") for f in to_modify):
+ return (False, "Should reuse existing constant, not create a new one")
+ return (True, "Correctly reuses existing constant")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: existing_constant_reuse | |
| description: Verify the skill reuses existing constants instead of creating duplicates | |
| check: | | |
| import json | |
| ann = outputs.get("annotations", {}) | |
| if not ann.get("expected_uses_existing_constant"): | |
| return (True, "Not a constant-reuse case") | |
| files = outputs.get("files", {}) | |
| modified = outputs.get("modified_files", {}) | |
| all_f = {**files, **modified} | |
| result_files = {k: v for k, v in all_f.items() if k.endswith("lint-fix-plan.json")} | |
| if not result_files: | |
| return (False, "No lint-fix-plan.json found") | |
| data = json.loads(list(result_files.values())[0]) | |
| full_text = json.dumps(data).lower() | |
| if "defaulttimeout" not in full_text: | |
| return (False, "Should reference existing constant DefaultTimeout") | |
| to_modify = data.get("files_to_modify", []) | |
| for f in to_modify: | |
| if f.get("reuses_existing_constant") is False: | |
| return (False, "Should reuse existing constant, not create a new one") | |
| return (True, "Correctly reuses existing constant") | |
| - name: existing_constant_reuse | |
| description: Verify the skill reuses existing constants instead of creating duplicates | |
| check: | | |
| import json | |
| ann = outputs.get("annotations", {}) | |
| if not ann.get("expected_uses_existing_constant"): | |
| return (True, "Not a constant-reuse case") | |
| files = outputs.get("files", {}) | |
| modified = outputs.get("modified_files", {}) | |
| all_f = {**files, **modified} | |
| result_files = {k: v for k, v in all_f.items() if k.endswith("lint-fix-plan.json")} | |
| if not result_files: | |
| return (False, "No lint-fix-plan.json found") | |
| data = json.loads(list(result_files.values())[0]) | |
| full_text = json.dumps(data).lower() | |
| if "defaulttimeout" not in full_text: | |
| return (False, "Should reference existing constant DefaultTimeout") | |
| to_modify = data.get("files_to_modify", []) | |
| if not any(f.get("reuses_existing_constant") for f in to_modify): | |
| return (False, "Should reuse existing constant, not create a new one") | |
| return (True, "Correctly reuses existing constant") |
🤖 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 `@plugins/golang/evals/eval-go-lint-fix.yaml` around lines 163 - 184, Update
the existing_constant_reuse check so it requires at least one files_to_modify
entry with reuses_existing_constant set to true, rather than failing when any
entry is false. Preserve the existing DefaultTimeout reference check and the
not-applicable behavior, allowing legitimate new constants such as MaxRetries in
the same file.
Comment on lines
+97
to
+187
| judges: | ||
| - name: valid_output_json | ||
| description: Verify output is valid JSON with required fields | ||
| check: | | ||
| import json | ||
| files = outputs.get("files", {}) | ||
| modified = outputs.get("modified_files", {}) | ||
| all_f = {**files, **modified} | ||
| result_files = {k: v for k, v in all_f.items() if k.endswith("lint-plan.json")} | ||
| if not result_files: | ||
| return (False, "No lint-plan.json found") | ||
| content = list(result_files.values())[0] | ||
| try: | ||
| data = json.loads(content) | ||
| except Exception as e: | ||
| return (False, f"Invalid JSON: {e}") | ||
| required = ["selected_command", "discovery_source", "would_modify_files"] | ||
| missing = [f for f in required if f not in data] | ||
| if missing: | ||
| return (False, f"Missing fields: {', '.join(missing)}") | ||
| return (True, "Valid JSON with required fields") | ||
|
|
||
| - name: discovery_command_correct | ||
| description: Verify the selected lint command matches the expected command from the discovery cascade | ||
| check: | | ||
| import json | ||
| ann = outputs.get("annotations", {}) | ||
| expected_cmd = ann.get("expected_command", "") | ||
| if not expected_cmd: | ||
| return (True, "No specific command expected") | ||
| files = outputs.get("files", {}) | ||
| modified = outputs.get("modified_files", {}) | ||
| all_f = {**files, **modified} | ||
| result_files = {k: v for k, v in all_f.items() if k.endswith("lint-plan.json")} | ||
| if not result_files: | ||
| return (False, "No lint-plan.json found") | ||
| data = json.loads(list(result_files.values())[0]) | ||
| selected = data.get("selected_command", "") | ||
| does_not_use = ann.get("expected_does_not_use", []) | ||
| errors = [] | ||
| if expected_cmd.lower() not in selected.lower(): | ||
| errors.append(f"Expected '{expected_cmd}', got '{selected}'") | ||
| for bad_cmd in does_not_use: | ||
| if bad_cmd.lower() in selected.lower(): | ||
| errors.append(f"Should NOT use '{bad_cmd}'") | ||
| if errors: | ||
| return (False, "; ".join(errors)) | ||
| return (True, f"Correct command: {selected}") | ||
|
|
||
| - name: read_only_constraint | ||
| description: Verify the skill never plans to modify files | ||
| check: | | ||
| import json | ||
| files = outputs.get("files", {}) | ||
| modified = outputs.get("modified_files", {}) | ||
| all_f = {**files, **modified} | ||
| result_files = {k: v for k, v in all_f.items() if k.endswith("lint-plan.json")} | ||
| if not result_files: | ||
| return (False, "No lint-plan.json found") | ||
| data = json.loads(list(result_files.values())[0]) | ||
| if data.get("would_modify_files", False): | ||
| return (False, "CONSTRAINT VIOLATION: go-lint is read-only, would_modify_files must be false") | ||
| return (True, "Read-only constraint respected") | ||
|
|
||
| - name: not_installed_handling | ||
| description: Verify proper handling when golangci-lint is not available — report, link, don't auto-install | ||
| check: | | ||
| import json | ||
| ann = outputs.get("annotations", {}) | ||
| if not ann.get("expected_reports_not_available"): | ||
| return (True, "Not a not-installed case") | ||
| files = outputs.get("files", {}) | ||
| modified = outputs.get("modified_files", {}) | ||
| all_f = {**files, **modified} | ||
| result_files = {k: v for k, v in all_f.items() if k.endswith("lint-plan.json")} | ||
| if not result_files: | ||
| return (False, "No lint-plan.json found") | ||
| data = json.loads(list(result_files.values())[0]) | ||
| action = str(data.get("not_installed_action", "")) | ||
| errors = [] | ||
| expected_url = ann.get("expected_install_url", "golangci-lint.run") | ||
| if expected_url and expected_url not in action: | ||
| errors.append(f"Missing install URL: {expected_url}") | ||
| if ann.get("expected_does_not_auto_install"): | ||
| auto_install_signals = ["go install", "brew install", "curl", "wget", "apt install", "pip install"] | ||
| for signal in auto_install_signals: | ||
| if signal in action.lower(): | ||
| errors.append(f"Must NOT auto-install, but found '{signal}'") | ||
| if errors: | ||
| return (False, "; ".join(errors)) | ||
| return (True, "Correct: reports not available with install link, no auto-install") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bind all declared case requirements to deterministic judges.
Several annotations are currently inert: expected_discovery_source, both cascade-order flags, expected_reports_issues, expected_suggests_lint_fix, and expected_does_not_fix are never checked. Also, read_only_constraint reads modified_files only to locate lint-plan.json; it never rejects source-file changes. A plan can therefore skip the cascade, edit files while reporting would_modify_files: false, or omit the lint-fix referral and still pass the deterministic checks.
Require discovery_steps, validate the expected source/order, validate issue reporting/referral when annotated, and fail if modified_files contains anything except lint-plan.json. Scan the complete plan for auto-install directives rather than only not_installed_action.
🤖 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 `@plugins/golang/evals/eval-go-lint.yaml` around lines 97 - 187, The
deterministic judges must enforce all declared annotations and read-only
behavior. Update valid_output_json to require discovery_steps, extend
discovery_command_correct to validate expected_discovery_source and both
cascade-order flags, and add checks for expected_reports_issues,
expected_suggests_lint_fix, and expected_does_not_fix. Strengthen
read_only_constraint to reject any modified_files entry other than
lint-plan.json, and scan the complete serialized plan for auto-install
directives in not_installed_handling.
Strengthen deterministic judges based on review feedback: - Clarify Path A/B strategy definitions to be mutually exclusive - Validate strategy and affected_modules against annotations - Check env vars per tidy/vendor command instead of full JSON search - Require actionable hard_stop_reason with relevant keywords - Use expected_action_word and actual ticket/CVE values in commit judge - Add expected_no_changes enforcement for not-affected cases - Fix existing_constant_reuse to allow new constants for MaxRetries - Add discovery_source_correct and suggests_lint_fix_skill judges - Add language identifier to README code fence Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
lifecycle/stale
Contributor
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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
1 participant
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
eval-fix-cve (6 cases, 8 judges)
// CVE-...\nreplace A => B v...formatgo mod vendorBEFOREmake updatefix(deps):prefix, ticket on own lineeval-go-lint (5 cases, 5 judges)
make verify-lint— use itmake lintgolangci-lint run ./...eval-go-lint-fix (4 cases, 6 judges)
// Code generatedheader → never modify//nolintDefaultTimeout, don't duplicategolangci-lint --fixdirectly, carry Makefile flagsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
nolint, constant reuse, directgolangci-lint --fixinvocation, and “not installed” handling), with plan validation and quality scoring.