feat(foreman): enforce reviewer execution runs via a no-test-run rail

[Defilan]

What

Make the reviewer rubric's execution steps enforceable. The two runs Section K asked for (run the diff's own new test; probe one adversarial near-miss) move into Step 1 as mandatory numbered items 7 and 8, and a deterministic rail (pkg/foreman/agent/review_execution_gate.go) checks the review transcript of any .go diff for a go test invocation. A GO with no such run is recorded as skipping the review-execution rail (no-test-run). The three reviewer Agent manifests are the make sync-reviewer-prompts output of the rubric change.

Why

Refs #1618

The first live Section-K review (wl-1616-added-lines-review-1616-0) performed every numbered read, skipped both execution runs, and disclosed the omission in onTrust. Numbered step sequences are followed faithfully; checklist prose is treated as advisory. So the mandate has to sit in the numbered sequence, and the transcript has to be checked by code rather than trusted.

How

The rail mirrors reviewer_diff_gate.go: walk the stored transcript, correlate tool results to the assistant tool call that produced them, return a finding, and record a skipped rail through the same recordRailSkipped mechanism (rail_skip.go gains railExecution and skipReasonNoTestRun). It is wired in executor_native.go directly after the diff gate so its verdict rewrite is what the findings summary reports. Non-.go diffs are exempt; a non-GO verdict is left alone. Tests cover: a Go diff whose transcript ran go test (GO stands), one that did not (rail recorded, verdict untouched, no demotion markers), a docs-only diff (exempt), a NO-GO input (untouched), a bash result correlated to a bash call (counts), a read_file result containing the literal (does not count), and a grep mention versus a cd && go test (only the latter counts).

The second commit is a Foreman revision pass driven by the human review below: the rail now marks instead of demoting (the verdict stands, the skipped rail is the record), and the transcript scan counts only executed bash commands, correlated through the same tool-call map the diff gate uses, with the regex anchored on a command position so a grep "go test" or a read_file of a test fixture cannot satisfy it.

Checklist

• Tests added/updated
• make test passes locally (verify gate GATE-PASS on the branch; CI green)
• make lint passes locally (CI)
• Commit messages follow conventional commits
• All commits are signed off (git commit -s) per DCO (currently signed by the Foreman bot; finalized with a human sign-off via scripts/foreman-finalize.sh before merge)
• AI assistance (if any) is disclosed above, per CONTRIBUTING.md
• Documentation updated (if user-facing change): the rubric itself

Assisted-by: Foreman (coder dsv4-flash-coder, DeepSeek-V4-Flash-Vision-Exp on vLLM) generated the code, tests and rubric edit from a decided design, then applied the human review findings in a second pass through the revise seam (#951); the verify gate ran the full check suite on both commits. The in-cluster reviewer's GO does not count either time: it captured no diff and ran no tests. Human review and the merge are mine.

[codecov]

Codecov Report

❌ Patch coverage is 90.62500% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/foreman/agent/review_execution_gate.go	90.32%	2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[Defilan]

Human review of the Foreman branch. Two changes needed before this leaves draft, one process note, one minor.

1. Demotion goes beyond what #1618 asked. The issue says "Marking, not demotion, until the fleet shows the runs fit the turn budget." enforceReviewerExecution demotes GO to NO-GO, and the PR's own rubric text says the opposite of its code: "A review of a Go diff whose transcript carries no go test is recorded as skipping the execution rail." The Workload intent that drove this run asked for demotion, so the coder followed its instructions; the intent was wrong, not the coder. With demotion live today, every reviewer that does not yet run tests (the review that opened this PR ran none in 6 turns) would turn every Go-diff GO into NO-GO and no PR would open. Please make the rail record-only: keep recordRailSkipped(extra, railExecution, skipReasonNoTestRun) and the log line, drop the verdictDemoted / verdictDemotedBy / verdictClaimed / demotionReason stamps and the NO-GO return, and adjust TestEnforceReviewerExecution_GoDiffNoGoTest to expect GO retained with the rail recorded. Update the executor_native.go comment and the file header to say marking. Flipping to demotion is a one-line follow-up once transcripts show the runs happening.

2. The transcript scan has a false positive that this very PR triggers. transcriptRanGoTest applies \bgo\s+test\b to the content of every tool-role message, so a read_file whose result contains the string satisfies the rail with nothing executed. review_execution_gate_test.go in this diff contains the literal go test ./pkg/foreman/agent/ -run TestReviewerSawDiff -count=1 in its fixtures, and Step 1 item 5 requires the reviewer to read every touched file, so a reviewer of this PR passes the rail by reading it. Only executed commands should count: assistant tool calls with Function.Name == "bash" (commandFromToolCallArgs on the arguments), and tool results correlated to such a call through callNameByToolCallID, which reviewer_diff_gate.go already builds. Drop the bare content match. Add a test where a tool result with Name read_file (or an uncorrelated tool message) containing go test yields false, alongside the existing TestTranscriptRanGoTest_ToolResultOnly, which should become "tool result correlated to a bash call".

3. The review that opened this PR carries no weight. The reviewer task ended ungroundedReview: true ("no diff --git line, no hunk header; the approval is uncorrelated with the code it covers"), its filesTouchedClaimed names two files not in the diff (charts/foreman/templates/crs-configmap.yaml, charts/foreman/tests/agent-rollout_test.yaml), and its APPROVE text was pasted into the original body. It is the case this rail exists for. Treat this thread as the review.

4. Minor. if extra == nil { return verdict } skips the rail silently; that mirrors the diff gate, so fine, but a log line would help the next person reading a transcript with no rail record. The regex also matches go test inside an unrelated command such as grep "go test" Makefile; anchoring on the command start (^\s*go\s+test\b or after &&/;/|) is cheap once only bash commands are scanned.

Finalization: the commit is signed by the Foreman bot. After the two fixes, scripts/foreman-finalize.sh --branch foreman/wl-1618-review-execution-gate/issue-1618 squashes to one human-signed commit and regenerates the reviewer Agent manifests.

[Defilan]

Re-review of the revision commit (70478bb). Approve.

Both findings are addressed as asked, and the revision touched only the three files the review named.

1. Mark, not demote. The four demotion stamps and the NO-GO return are gone; recordRailSkipped and a log line remain and the verdict is returned as the model gave it. The file header, the function doc, and the executor_native.go comment all describe marking, and now agree with the rubric sentence. TestEnforceReviewerExecution_GoDiffNoGoTest asserts GO retained, the rail recorded with no-test-run, and the absence of every demotion key.

2. Executed bash only. Assistant tool calls count only when Function.Name == "bash". Tool results count only when callNameByToolCallID maps them to a bash call, and the match runs on the result's parsed command field via commandFromToolCallArgs, which returns empty for non-JSON content, so a bash stdout that happens to contain the words cannot match either. The regex is anchored on a command position, so grep "go test" Makefile does not count while cd /tmp && go test ./... does. New tests cover the correlated bash result, the read_file of a fixture containing the literal, and the grep-versus-&& pair. Nil extra now logs.

Ran go test ./pkg/foreman/agent/ (full package), go vet, and gofmt -l on the branch: clean. CI green on both commits.

Process note, same as the first round: the in-cluster review of this commit was again marked ungrounded by its own rails (no diff captured, 7 turns, no test run). Once this rail is deployed, that review would carry review-execution: no-test-run in its record, which is the whole point.

Ready to leave draft; squash-merge keeps the two Foreman commits as one, as with #1741 and #1751.