diff --git a/agents/glue-review/CHANGELOG.md b/agents/glue-review/CHANGELOG.md index 0a6e63a..7bfab83 100644 --- a/agents/glue-review/CHANGELOG.md +++ b/agents/glue-review/CHANGELOG.md @@ -4,6 +4,34 @@ This file tracks releases of the `glue-review` agent and its GitHub Action. It is independent from the parent `github.com/erain/glue` library, which versions separately. +## v1.1.0 ā€” 2026-05-06 + +Backwards-compatible feature release. Existing v1.0.0 deployments +continue to work without changes. + +### Added + +- **Inline AI-fix prompts.** Each `[severity] path:line` entry now + includes a `Fix: ` clause. The Action renders it as a + `
šŸ’” AI prompt to fix` collapsible inside + every inline review comment, so reviewers can copy the prompt + directly into Claude / Cursor / their coding agent of choice. + Inspired by the [fluent-bit project's review style](https://github.com/fluent/fluent-bit/pull/11778#pullrequestreview-4236889315). +- **Prompt v2** ā€” the default prompt version is now `v2`, which + requires the `Fix:` clause. The legacy `v1` prompt remains in + source; pin via `--prompt-version v1` (or the matching Action input) + to opt back in if a deployment relies on the v1 output shape. +- `InlineComment.fix` JSON field. Empty when the model produced a v1- + style entry without a Fix clause; populated for v2+ entries. + +### Unchanged + +- Action input/output surface ā€” additive only. +- Sticky comment / PR Review marker shape ā€” the Fix collapsible is + optional content within the existing inline-comment shape. +- v1 prompt output continues to flow through the parser and + validator unchanged. + ## v1.0.0 ā€” 2026-05-05 First stable release. The Action's input/output surface is now diff --git a/agents/glue-review/README.md b/agents/glue-review/README.md index 00272bd..d686c68 100644 --- a/agents/glue-review/README.md +++ b/agents/glue-review/README.md @@ -145,6 +145,38 @@ each path component (case-insensitive), so `infra/secrets/foo.yaml` is caught by the pattern `secrets`. You cannot subtract a default ā€” only add. +## AI fix prompts (v1.1+) + +Each inline review comment carries a copy-pastable instruction +targeted at a coding agent, rendered as a `
` collapsible: + +```markdown +**[major]** Off-by-one error in loop bound + +
šŸ’” AI prompt to fix + +``` +In math.go, change the loop in SumFirstN from `for i := 0; i <= n; +i++` to `for i := 1; i <= n; i++` so it sums 1..n inclusive. +``` + +
+``` + +Reviewers copy the inner block into Claude / Cursor / Aider / etc. to +dispatch the fix without retyping. Pattern inspired by +[fluent-bit's review style](https://github.com/fluent/fluent-bit/pull/11778#pullrequestreview-4236889315). + +To turn off (revert to plain inline comments without the +collapsible), pin the prompt to v1: + +```yaml +- uses: erain/glue/agents/glue-review@v1 + with: + prompt-version: v1 + nvidia-api-key: ${{ secrets.NVIDIA_API_KEY }} +``` + ## Prompt versioning The system prompt lives at [`prompts/v1.md`](prompts/v1.md), embedded diff --git a/agents/glue-review/action.yml b/agents/glue-review/action.yml index 3ae6a8e..2703a41 100644 --- a/agents/glue-review/action.yml +++ b/agents/glue-review/action.yml @@ -228,9 +228,16 @@ runs: head_sha=$(gh api "repos/$REPO/pulls/$PR_NUMBER" --jq '.head.sha') # Dismiss any prior inline reviews from this bot before posting a - # new one. Match by the v1 marker we inject into every review body + # new one. Match by the marker we inject into every review body # so re-runs replace, not stack. Dismissing keeps the audit trail # but greys out old reviews in the UI. + # + # NOTE on the `promptv1` token in the marker: this is a stable + # bot-identity tag, not a strict prompt-version tracker. We + # only bump it when the COMMENT SHAPE changes in a way that + # would mangle an in-place edit. The Fix: collapsible added in + # prompt v2 is additive (optional content inside the existing + # comment shape), so the marker stays at v1. prior_ids=$( gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/reviews" \ --jq "[.[] | select(.body and (.body | contains(\"\"))) | select(.state != \"DISMISSED\") | .id] | .[]" \ @@ -255,12 +262,22 @@ runs: # PR Reviews API expects comments as `{path, line, side, body}`. # Severity is folded into the body so reviewers see it inline. + # When the agent produced a Fix: clause (prompt v2+), it is + # rendered as a
collapsible carrying a copy-pastable + # AI-coding-agent prompt ā€” letting the reviewer dispatch a fix + # with one click. Entries without a Fix: clause render exactly + # as before, so v1 inline-comment payloads stay valid. comments_path="${RUNNER_TEMP}/review-comments.json" jq '[.[] | { - path: .path, - line: .line, - side: "RIGHT", - body: "**[\(.severity)]** \(.body)" + path: .path, + line: .line, + side: "RIGHT", + body: ( + "**[\(.severity)]** \(.body)" + + (if (.fix // "") == "" then "" + else "\n\n
šŸ’” AI prompt to fix\n\n```\n\(.fix)\n```\n\n
" + end) + ) }]' "$INLINE_PATH" > "$comments_path" # Build the full review payload as JSON so newlines / quotes survive. @@ -271,17 +288,39 @@ runs: '{commit_id: $commit_id, body: $body, event: "COMMENT", comments: $comments[0]}' \ > "$payload_path" - # Post. If the API rejects (e.g., a path:line not in the diff), - # capture the error and fall back to the sticky path. - if url=$(gh api -X POST "repos/$REPO/pulls/$PR_NUMBER/reviews" \ - --input "$payload_path" --jq '.html_url'); then - echo "Posted inline review: $url" - echo "posted=true" >> "$GITHUB_OUTPUT" - echo "review-url=$url" >> "$GITHUB_OUTPUT" - else - echo "Inline review POST failed; will fall back to sticky comment." - echo "posted=false" >> "$GITHUB_OUTPUT" - fi + # Post. The PR Reviews API occasionally returns 422 with a + # generic "An internal error occurred, please try again." + # message that has nothing to do with the payload. Retry up + # to 3 times with backoff before falling back to the sticky + # comment path. The full API response is dumped on the final + # failure so the workflow log is debuggable without re-running. + api_resp_path="${RUNNER_TEMP}/review-api-response.json" + attempt=0 + max_attempts=3 + while : ; do + attempt=$((attempt + 1)) + if gh api -X POST "repos/$REPO/pulls/$PR_NUMBER/reviews" \ + --input "$payload_path" > "$api_resp_path" 2>&1; then + url=$(jq -r '.html_url' "$api_resp_path") + echo "Posted inline review: $url (attempt $attempt)" + echo "posted=true" >> "$GITHUB_OUTPUT" + echo "review-url=$url" >> "$GITHUB_OUTPUT" + break + fi + if [ "$attempt" -ge "$max_attempts" ]; then + echo "Inline review POST failed after $attempt attempts; falling back to sticky comment." + echo "----API response (last 4k)----" + tail -c 4096 "$api_resp_path" || true + echo + echo "----submitted comments (first 20)----" + jq '.[0:20] | map({path, line, side, body_chars: (.body | length)})' "$comments_path" || true + echo "posted=false" >> "$GITHUB_OUTPUT" + break + fi + echo "Inline review POST failed on attempt $attempt; retrying in $((attempt * 2))sā€¦" + tail -c 512 "$api_resp_path" || true + sleep $((attempt * 2)) + done - name: Post or update sticky comment id: post diff --git a/agents/glue-review/parse.go b/agents/glue-review/parse.go index 37c521a..45ab2ae 100644 --- a/agents/glue-review/parse.go +++ b/agents/glue-review/parse.go @@ -8,11 +8,20 @@ import ( // InlineComment is one line-level review entry that the calling Action // can submit via the GitHub Pull Request Reviews API. +// +// Fix is optional and populated when the model included a +// `Fix: ` clause after the body (prompt v2+). The Action +// renders it as a collapsible "AI prompt to fix" block in the inline +// comment so reviewers can copy-paste it into a coding agent. v1 +// prompts emit no Fix and the field stays empty ā€” the Action skips +// the collapsible in that case, so behavior is fully backwards- +// compatible with v1 inline-comment payloads on disk. type InlineComment struct { Path string `json:"path"` Line int `json:"line"` Severity string `json:"severity"` Body string `json:"body"` + Fix string `json:"fix,omitempty"` } // inlineEntryRE matches a single Issues / Suggestions list entry the @@ -58,11 +67,13 @@ func parseInlineComments(markdown string) []InlineComment { if err != nil || line <= 0 { continue } + b, fix := splitBodyAndFix(strings.TrimSpace(m[4])) out = append(out, InlineComment{ Severity: m[1], Path: m[2], Line: line, - Body: strings.TrimSpace(m[4]), + Body: b, + Fix: fix, }) } } @@ -109,3 +120,29 @@ func matchHeading(line string) (string, bool) { } return strings.TrimSpace(trimmed[3:]), true } + +// fixDelimiterRE matches the ` Fix: ` separator the prompt v2 instructs +// the model to inject between the description and the AI agent prompt. +// Tolerates leading whitespace and case variation (`fix:`, `FIX:`), +// since model output drifts on case-sensitive markers. +var fixDelimiterRE = regexp.MustCompile(`(?i)[. ]\s*\bfix\s*:\s*`) + +// splitBodyAndFix separates `description. Fix: agent prompt` into its +// two parts. Returns (body, "") when no Fix clause is present, so v1 +// prompts continue to flow through unchanged. +// +// Uses the LAST match of the Fix delimiter, since the description +// itself can legitimately contain words like "fix" (e.g., "the fix +// should be obvious"). The model's `Fix:` clause is by convention the +// last clause in the entry, so the last match is the right one. +func splitBodyAndFix(s string) (body, fix string) { + locs := fixDelimiterRE.FindAllStringIndex(s, -1) + if len(locs) == 0 { + return strings.TrimSpace(s), "" + } + loc := locs[len(locs)-1] + body = strings.TrimSpace(s[:loc[0]+1]) + fix = strings.TrimSpace(s[loc[1]:]) + body = strings.TrimRight(body, ".") + return strings.TrimSpace(body), fix +} diff --git a/agents/glue-review/parse_test.go b/agents/glue-review/parse_test.go index 790fbbb..298983a 100644 --- a/agents/glue-review/parse_test.go +++ b/agents/glue-review/parse_test.go @@ -73,6 +73,84 @@ func TestParseInlineCommentsFreeFormPasses(t *testing.T) { } } +func TestParseInlineCommentsCapturesFix(t *testing.T) { + t.Parallel() + md := "## Issues\n" + + "- [major] math.go:6 ā€” Off-by-one bug. Fix: change `i <= n` to `i < n`.\n" + + "- [minor] util.go:9 ā€” Missing nil check. Fix: guard with `if x == nil { return }`.\n" + got := parseInlineComments(md) + if len(got) != 2 { + t.Fatalf("len = %d, want 2; got=%+v", len(got), got) + } + if got[0].Body != "Off-by-one bug" || got[0].Fix != "change `i <= n` to `i < n`." { + t.Errorf("entry 0: body=%q fix=%q", got[0].Body, got[0].Fix) + } + if got[1].Body != "Missing nil check" || got[1].Fix != "guard with `if x == nil { return }`." { + t.Errorf("entry 1: body=%q fix=%q", got[1].Body, got[1].Fix) + } +} + +func TestParseInlineCommentsBackcompatNoFix(t *testing.T) { + t.Parallel() + // v1 prompt output: no Fix clause. Body stays whole, Fix is empty. + md := "## Issues\n- [major] foo.go:3 ā€” Something is wrong here.\n" + got := parseInlineComments(md) + if len(got) != 1 { + t.Fatalf("len = %d, want 1", len(got)) + } + if got[0].Body != "Something is wrong here." || got[0].Fix != "" { + t.Errorf("body=%q fix=%q (want body with period, fix empty)", got[0].Body, got[0].Fix) + } +} + +func TestParseInlineCommentsFixUsesLastMatch(t *testing.T) { + t.Parallel() + // "fix" appearing in the description must NOT cause a premature + // split. The real `Fix:` is the LAST one. + md := "## Issues\n- [major] x.go:1 ā€” The fix should be obvious. Fix: rewrite the function.\n" + got := parseInlineComments(md) + if len(got) != 1 { + t.Fatalf("len=%d", len(got)) + } + if got[0].Body != "The fix should be obvious" { + t.Errorf("body lost the in-description 'fix': got %q", got[0].Body) + } + if got[0].Fix != "rewrite the function." { + t.Errorf("fix wrong: %q", got[0].Fix) + } +} + +func TestSplitBodyAndFixVariations(t *testing.T) { + t.Parallel() + cases := []struct { + in string + wantBody string + wantFix string + }{ + // Standard form. + {"do X. Fix: do Y", "do X", "do Y"}, + // Lowercase variant. + {"do X. fix: do Y", "do X", "do Y"}, + // All-caps. + {"do X. FIX: do Y", "do X", "do Y"}, + // No period, just space. + {"do X Fix: do Y", "do X", "do Y"}, + // No Fix at all ā€” body kept whole, including any trailing period + // (matches the v1 backwards-compat path). + {"do X.", "do X.", ""}, + {"do X", "do X", ""}, + // "fix" used in body ā€” last match wins. + {"the fix is small. Fix: real prompt", "the fix is small", "real prompt"}, + } + for _, c := range cases { + body, fix := splitBodyAndFix(c.in) + if body != c.wantBody || fix != c.wantFix { + t.Errorf("splitBodyAndFix(%q) = (%q, %q), want (%q, %q)", + c.in, body, fix, c.wantBody, c.wantFix) + } + } +} + func TestSplitSections(t *testing.T) { t.Parallel() md := "## Summary\nintro\n\n## Issues\nfirst\n\n## Suggestions\nsecond\n" diff --git a/agents/glue-review/prompt.go b/agents/glue-review/prompt.go index 0213126..32a0c98 100644 --- a/agents/glue-review/prompt.go +++ b/agents/glue-review/prompt.go @@ -19,7 +19,13 @@ var systemPromptsFS embed.FS // defaultPromptVersion picks the prompt loaded when --prompt-version is // empty. Bump only when you want every consumer to see the new prompt // without an explicit opt-in. -const defaultPromptVersion = "v1" +// +// History: +// - v1 ā€” initial prompt (1.0 release). +// - v2 ā€” adds `Fix: ` at the end of every Issues / +// Suggestions entry, so the Action can render a copy-pastable +// coding-agent prompt next to each inline comment. +const defaultPromptVersion = "v2" // systemPromptFor returns the embedded prompt for the requested // version. An unknown version returns an error listing the available diff --git a/agents/glue-review/prompts/v2.md b/agents/glue-review/prompts/v2.md new file mode 100644 index 0000000..4c71716 --- /dev/null +++ b/agents/glue-review/prompts/v2.md @@ -0,0 +1,37 @@ +You are a senior software engineer reviewing a Git branch before it is pushed for review. + +Workflow: +1. Call git_diff_branch first to see the full diff against the base branch. +2. Call git_log_branch to see the commit history; this often explains intent. +3. For files where the diff alone is not enough context (large refactors, new files, subtle invariants), call read_file on specific paths to look at the surrounding code. Skim purposefully ā€” do not read every file. +4. Emit a single, final review. Do not chat between tool calls. + +Output format (Markdown, in this order, omit empty sections): + +## Summary +One sentence on what this branch does. + +## Issues +Bugs, regressions, or correctness problems. Each entry MUST follow this exact shape so a tool can route it as an inline review comment with an attached AI-agent fix prompt: + +- [critical|major|minor] path/to/file.ext:LINE ā€” description. Fix: a self-contained, copy-pastable instruction a coding agent can act on. + +Where: +- LINE is the line number on the new (post-change) side of the diff. If you genuinely cannot pin a line, use :0 (the entry will land in the bulk review body instead of inline). +- The text after `Fix: ` is a single complete instruction ā€” present tense, imperative, naming the file and the change concretely. Do NOT include narrative ("I think you should...") ā€” write it as the prompt you would give an AI coding agent. The fix prompt may be long; do not truncate it for brevity. + +Example: +- [major] math.go:6 ā€” Off-by-one: loop runs from 0..n inclusive, which sums n+1 terms instead of n. Fix: In math.go, change the loop in SumFirstN from `for i := 0; i <= n; i++` to `for i := 1; i <= n; i++` so it sums 1..n inclusive as the doc comment states. + +## Suggestions +Style, design, or maintainability improvements. Same prefix and `Fix:` shape as Issues: + +- [minor|major] path/to/file.ext:LINE ā€” description. Fix: concrete instruction. + +## Looks good +Free-form bullets. Only when meaningful ā€” do not pad. No `Fix:` here. + +## Open questions +Things you cannot decide from the diff alone and want the author to clarify. Free-form bullets. No `Fix:` here. + +Be direct. Never invent code that is not in the diff. Never reference files that are not changed by the diff in Issues or Suggestions. Always include `Fix: ` after the description in Issues and Suggestions.