fix(foreman): guard issue rendering in reviewer prompt when issue is unset

[snowyukitty]

Problem

When a pipeline dispatch payload omits the issue key (or passes issue: 0), buildUserPrompt on AgenticTaskKindReview rendered You are reviewing the branch the coder produced for issue #0 of <repo> and - issue: 0. This instructed reviewer models to evaluate an issue number that does not exist (#1570).

Solution

• In pkg/foreman/agent/executor_native.go, check p.Issue > 0 before emitting issue references in the review prompt header; when Issue is 0 or unset, render You are reviewing the branch <branch> of <repo>. without referencing issue #0.
• Add unit test TestBuildUserPrompt_ReviewerCase_ZeroIssue_OmitsIssueLine in pkg/foreman/agent/executor_native_internal_test.go verifying that unset or zero issues omit issue lines.

Fixes #1570

[snowyukitty]

Before this is merged, a correction to my own description.

Fixes #1570 is wrong. #1570 is "Reviewer returns GO without ever diffing the branch: verdicts uncorrelated with the code they approve". This PR does not address that — it stops issue #0 and - issue: 0 being rendered into the reviewer prompt when the dispatch payload carries no issue. That is a separate defect, not the bug #1570 reports.

#1588 ("rail detecting a GO from a reviewer that never diffed", merged 2026-08-18) uses Refs #1570, and #1570 is still open. The closing keyword here is in both this description and the commit message, so merging as written would close it. I wanted to correct that before merge so #1570 stays open.

The defect this PR does fix is still present on main: in pkg/foreman/agent/executor_native.go, the AgenticTaskKindReview branch of buildUserPrompt still emits both issue lines unconditionally, while repoMapQuery in the same file already uses the if p.Issue > 0 guard.

Happy to reword the description and the commit message to Refs #1570, and to rebase and re-run the tests, if either would be useful.

[Defilan]

Thanks for this, and welcome. The code change is correct and I want it: guarding on p.Issue > 0 before emitting the issue references is the right shape, the fallback wording reads well, and it comes with a test. My concerns are about what the PR claims and how it is filed, not about the change itself.

Blocking: this does not fix #1570, and merging it would close that issue

#1570 reports that an in-process reviewer returns GO without ever obtaining the diff of the branch it is reviewing. Its Expected Behavior is explicit:

A reviewer that never diffs the branch under review cannot return GO. Either the loop makes the diff unavoidable, or a rail fails the verdict when the transcript shows the branch was never diffed.

This PR does neither. After it merges, a reviewer can still read an unrelated file, never diff the branch, and return GO. The verdict stays uncorrelated with the code it approves, which is the whole of that bug.

I think I can see how this happened. Step 1 of #1570's reproduction says to dispatch a review step "whose payload carries repo, branch, baseBranch and branchStrategy but no issue key." That missing key is the trigger condition I used to reproduce the ungrounded review, not the defect. Rendering issue #0 is a real and separate bug that you found while reading the same code.

Because the body says Fixes #1570, merging this auto-closes that issue. A severe correctness bug would vanish from the backlog while remaining entirely present in the code, and #1570 is arguably the strongest case in epic #1548. That is the one thing here I cannot merge past.

Please change Fixes #1570 to Refs #1570, and open a separate issue for the issue #0 rendering so the fix has its own tracking. Happy to review that issue quickly.

Blocking: the PR template is required

This repository has a mandatory PR template at .github/PULL_REQUEST_TEMPLATE.md, and this description does not use it. None of the What / Why / How / Checklist sections are present and the checklist is absent, so I cannot see the state of the DCO sign-off, the local make test and make lint runs, or the documentation question.

Your Problem / Solution write-up is genuinely clearer than a lot of what the template produces, so please keep that content. It maps cleanly: Problem becomes Why, Solution becomes How. What you would be adding is the checklist and a one-line What.

While filling it in, please answer the AI-assistance item explicitly. CONTRIBUTING.md asks that if any part of a contribution was generated or substantially assisted by an AI tool, the PR body says which tool, what it produced, and what you verified before submitting. We welcome AI-assisted contributions and hold them to the same bar as any other; we just need the disclosure, and a human owning the review conversation. If no tool was involved, say that and we move on.

I am raising it because the scope mismatch above is the shape a tool produces when pointed at an issue: a correct, well-formed change to something adjacent to the reported defect, described as fixing it. That is a normal failure mode and not a criticism of the work, but it is exactly why the disclosure matters, since it tells a reviewer where to look hardest.

Non-blocking, on the change itself

Worth covering the same guard on the coder path, or saying why not. This fixes the AgenticTaskKindReview branch of buildUserPrompt. If the coder branch renders issue references the same way, an issueless pipeline step hits the identical #0 there. Either extend the guard or note in the PR that the coder path always has an issue and cannot reach it.

One question about the test. Does TestBuildUserPrompt_ReviewerCase_ZeroIssue_OmitsIssueLine fail if the guard is removed? Worth confirming rather than assuming: this codebase has shipped several tests that passed whether or not the code under test worked, and the cheap check is to delete the if p.Issue > 0 guard, run the test, and see it go red. If it does, say so in the PR and I will take your word for it.

Negative assertions are the easy ones to get wrong here. If the test asserts the output does not contain issue #0, make sure it also asserts the fallback line it should contain. A test that only checks for an absence passes against an empty string too.

To summarise

Two things to change before merge: Fixes to Refs with a separate issue for the real defect, and the PR template with the AI-assistance item answered. Then this is a good small fix and I will merge it.

Sorry to hand a first contribution a request for changes over bookkeeping rather than code. The code is fine. It is the issue link that would do damage.