feat(code-agent): use PR template for pr_body in structured output#56
feat(code-agent): use PR template for pr_body in structured output#56rh-hemartin wants to merge 1 commit into
Conversation
PR Summary by QodoUse repo PR templates to populate structured pr_body for PR creation
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
🤖 Finished Review · ❌ Failure · Started 7:24 AM UTC · Completed 7:38 AM UTC |
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Protected paths modified (scripts/, skills/)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsMedium
Low
Prior review resolution
Previous runReviewVerdict: approve · 2 low findings Clean feature addition. The prior medium-severity finding (step 11 in SKILL.md contradicting the new The architecture follows the established structured-output pattern cleanly: agent writes The awk footer-stripping logic is more comprehensive than the legacy path: it handles Test coverage is solid: 8 new test cases cover the happy path, verbatim pass-through (no unwrapping), strip-to-empty fallback, cross-repo Closes stripping, content-embedded Closes preservation, and Fixes/Resolves keyword variants. Findings1. [low] [test-coverage]
|
| # | Prior finding | Status |
|---|---|---|
| 1 | [medium] Step 11 stale instructions | ✅ Resolved — text updated to reflect both fields |
| 2 | [low] agents/code.md incomplete description |
✅ Resolved — structured output section updated |
| 3 | [low] Test coverage gaps | ✅ Mostly resolved — Fixes/Resolves/cross-repo tests added; one minor gap remains (finding 1 above) |
| 4 | [low] Test reimplements production logic | ➡ Unchanged — established pattern in this file |
Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.Protected files in this PR:
agents/code.mdscripts/post-code-test.shscripts/post-code.shskills/code-implementation/SKILL.md
Previous run (2)
Review
Verdict: comment · 1 medium, 3 low findings
Clean feature migration. The architecture (agent writes pr_body to code-result.json, post-script reads it verbatim, falls back to commit body) fits the existing structured-output pattern. Schema change is backward compatible (additive optional field), shell handling is safe (printf '%s\n' + sed pipeline), and the test additions follow the established test-helper reimplementation pattern in post-code-test.sh. The jq error-suppression idiom matches the existing AGENT_TARGET extraction on line 80.
Findings
1. [medium] [stale-instructions] skills/code-implementation/SKILL.md · lines 780–790
Step 11 text contradicts the new pr_body field. Step 11 currently states: "The file must be valid JSON with exactly one field" and "Only the target_branch field is allowed. Any extra fields will cause validation to fail." Since this PR adds pr_body as a valid optional field in the schema, those statements are now factually wrong. An agent following step 11 literally may remove pr_body from the output before running fullsend-check-output, silently defeating the feature.
The risk is mitigated by the fact that step 3 (newly updated) explicitly instructs the agent to write pr_body, and fullsend-check-output will pass with pr_body present. However, the contradictory text creates unnecessary ambiguity.
Remediation: Update step 11 to reflect the new schema — change "exactly one field" to "the required field (target_branch) and optionally pr_body", update the JSON example to show both fields, and revise the compliance note.
2. [low] [stale-documentation] agents/code.md · lines 97–100
The structured output section says the file "documents the target branch for PR creation" and the post-script "reads this file to determine which branch to target the PR against." Now that the file also carries pr_body, this description is incomplete. The schema and SKILL.md are the authoritative references, so impact is minimal.
3. [low] [test-coverage] scripts/post-code-test.sh
The new pr_body tests cover the happy path (content present, verbatim pass-through, Closes deduplication) but omit several edge cases: (a) pr_body that becomes empty after Signed-off-by / Closes stripping — should fall back to the automated description via the existing if [ -z "${COMMIT_BODY}" ] guard; (b) pr_body with cross-repo closes (Closes org/repo#N); (c) pr_body with trailing blank lines. These are handled by existing fallback logic, so the risk is low.
4. [low] [code-organization] scripts/post-code-test.sh · line 127
build_pr_body() in the test file reimplements the production inline logic from post-code.sh. The sed pipelines currently match exactly. This follows the established pattern throughout post-code-test.sh (8+ helpers reimplement production logic for isolation testing), so it is consistent — but the duplication remains a future maintenance consideration.
Notes
- The Closes-stripping sed patterns only handle the
Closeskeyword (notFixes,Resolves,Fix, etc.). This is a pre-existing limitation shared with the legacy commit-body path and is not introduced by this PR. The PR actually improves coverage by adding cross-repo Closes stripping in the new path. - The asymmetry between the legacy path (
sed '/^Closes #/d') and the new path (which adds cross-repo pattern) is an intentional improvement, not a regression. - The schema change is fully backward compatible:
pr_bodyis optional, not in therequiredarray, andadditionalProperties: falseis maintained. Existing outputs with onlytarget_branchcontinue to validate. - Shell safety is sound:
printf '%s\n'prevents backslash interpretation, double-quoting prevents word splitting, andsedreads from stdin. No injection vector exists in the new data path.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-squad pass (5 agents: Claude, Gemini, Codex) on top of the existing qodo-code-review findings. Posted 6 inline comments for medium+ issues not already covered by qodo's 4 comments (protected-paths, jq-failure-silencing, docs-contradiction, over-broad-stripping).
One additional MEDIUM finding couldn't be attached inline because it's on an unchanged line outside the diff:
[MEDIUM] Step 6's "Label-gated" early exit has no fullsend-check-output call (skills/code-implementation/SKILL.md, the "Label-gated" bullet under step 6, "Identify the task type"). The prior step 3 had an explicit instruction that any early exit (steps 4, 6, or 7) must run fullsend-check-output before exiting; this PR's diff removes that block. Steps 4, 7, 9c, and 11 each have their own inline call, and the "Already-fixed" bullet routes through step 7's call — but "Label-gated" has no call of its own and previously relied on the now-removed step 3 guidance. Suggest adding an explicit fullsend-check-output "${FULLSEND_OUTPUT_DIR}/code-result.json" call to that bullet.
No shell/command-injection issues found — verified gh pr create --body handling is safe against $()/backtick payloads in pr_body, and pr_body never reaches GITHUB_OUTPUT.
rh-hemartin
force-pushed
the
feat/pr-body
branch
from
f828ec7 to
0c74ec6
Compare
|
🤖 Finished Review · ✅ Success · Started 7:00 AM UTC · Completed 7:08 AM UTC |
fullsend-ai-review
Bot
added
the
requires-manual-review
Teach the code agent to discover repo PR templates and write a template-structured pr_body field in code-result.json. The post-script uses pr_body verbatim as the PR description, falling back to the commit body when absent. Changes: - Add optional pr_body field to code-result.schema.json (maxLength: 65536) - Add pr_body reading logic to post-code.sh with printf-safe piping - Tighten Closes-line stripping to only match GitHub ref forms - Add PR template discovery step to SKILL.md (step 3 item 5) - Replace heredoc examples with jq -n pattern (safe from shell expansion) - Add pr_body test cases to post-code-test.sh Migrated from fullsend-ai/fullsend#2979. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
rh-hemartin
force-pushed
the
feat/pr-body
branch
from
0c74ec6 to
ee18489
Compare
|
🤖 Finished Review · ✅ Success · Started 7:17 AM UTC · Completed 7:33 AM UTC |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| use it when writing the commit subject in step 10. | ||
| 5. **Check for PR template.** Look for `.github/pull_request_template.md`, | ||
| `.github/PULL_REQUEST_TEMPLATE.md`, or `docs/pull_request_template.md`. | ||
| If a directory `.github/PULL_REQUEST_TEMPLATE/` exists with multiple |
There was a problem hiding this comment.
[low] prompt-injection-surface
Step 3 item 5 instructs the code agent to read PR template files from target repos. A malicious template could contain adversarial instructions. Risk is bounded by defense-in-depth and is consistent with the existing threat surface from reading CLAUDE.md and CONTRIBUTING.md from untrusted repos.
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-squad pass (3 agents: Claude, Claude, Gemini) on top of the existing qodo-code-review and fullsend-ai-review bot history. All prior findings were re-verified against the live file content before this pass (three from the prior ad hoc review-squad round turned out to be false positives — ${FULLSEND_OUTPUT_FILE} undefined, ${pr_template} undefined, and the docs-say-"exactly one field" claim — none reproduce against the current file). Posting the unique medium-and-above findings from this round below as inline comments.
Most notable: a CRITICAL, empirically-reproduced command-injection bug in the step 10a jq --arg merge example, which slipped past three prior review rounds — including this repo's own deployed fullsend-ai-review bot explicitly asserting twice that "jq --arg avoids shell expansion." I verified this live: substituting a backtick-quoted command into the documented pattern causes bash to execute it before jq ever sees the string.
Also new: a HIGH finding that pr_body never passes through gitleaks/secret scanning (it lives under $FULLSEND_OUTPUT_DIR, outside the git tree gitleaks detect --source . scans), and a HIGH finding that pr_body's maxLength (65536) exactly matches GitHub's hard body-length ceiling with no headroom for the footer post-code.sh always appends.
| shell expansion of `$` and backticks in markdown content: | ||
|
|
||
| ```bash | ||
| jq --arg pb "<pr_body content>" '. + {pr_body: $pb}' \ |
There was a problem hiding this comment.
[CRITICAL] jq --arg merge command is vulnerable to shell command substitution, despite the doc's own safety claim
The surrounding text (line 786) claims this pattern "avoids shell expansion of $ and backticks in markdown content." I verified live that it does not: substituting real pr_body content containing a backtick-quoted command (e.g. `npm test` in a Testing section) into jq --arg pb "<pr_body content>" causes bash to execute the backtick expression before jq ever sees the string — the command's output silently ends up baked into pr_body instead of the literal text. jq --arg only prevents JSON-escaping issues; it does nothing for shell parsing of the double-quoted argument it's embedded in.
Suggestion: Populate a variable via a quoted heredoc first (which disables expansion), then pass that variable to jq:
PR_BODY_CONTENT=$(cat <<'EOF'
<pr_body content>
EOF
)
jq --arg pb "$PR_BODY_CONTENT" '. + {pr_body: $pb}' ...Also correct the false "avoids shell expansion" claim in the prose.
| ')" | ||
|
|
||
| # Read pr_body from agent output. Fall back to commit body if absent. | ||
| PR_BODY_FROM_RESULT="" |
There was a problem hiding this comment.
[HIGH] pr_body is never secret-scanned before becoming a public PR description
gitleaks detect --source . (line 246) scans the git working tree, but code-result.json (where pr_body lives) is written under $FULLSEND_OUTPUT_DIR, a directory outside the git repo tree entirely (per the runner docs, repo checkout is /tmp/workspace/repo while output is /tmp/workspace/output). pr_body is free-text up to 65536 chars the agent writes directly there — it's never staged, never committed, and therefore never crosses the "Authoritative secret scan — final gate before any push" that every other piece of agent-written content passes through. A secret pasted into the description (from a log, config, or prompt-injected template) reaches a public PR body unguarded.
Suggestion: Run gitleaks (or equivalent) against the raw pr_body string here before using it, and hard-fail/fall back to the commit body if a secret is detected — mirroring the existing Signed-off-by hard-block pattern at lines 257-264.
| }, | ||
| "pr_body": { | ||
| "type": "string", | ||
| "maxLength": 65536, |
There was a problem hiding this comment.
[HIGH] maxLength exactly matches GitHub's hard body-length limit, leaving no headroom for the footer post-code.sh always appends
65536 is GitHub's exact PR/issue body character ceiling. post-code.sh (lines 515-526) unconditionally appends a ~150-250 char footer (---, Closes #N, verification checklist) after the agent's pr_body. A schema-valid pr_body near the ceiling pushes the assembled body over GitHub's limit, so gh pr create fails outright with no automated recovery path.
Suggestion: Lower maxLength to reserve headroom for the footer (e.g. ~65000 minus a safety margin), or validate/truncate the assembled body length in post-code.sh before calling gh pr create.
| end = NR | ||
| while (end > 0) { | ||
| l = lines[end] | ||
| if (l == "" || l ~ /^Signed-off-by:/ || l ~ /^[Cc]lose[sd]? (#|[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+#)/ || l ~ /^[Ff]ix(e[sd])? (#|[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+#)/ || l ~ /^[Rr]esolve[sd]? (#|[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+#)/) |
There was a problem hiding this comment.
[MEDIUM] Footer-stripping regex is not end-anchored — can silently delete real trailing content, not just the closing reference
Each alternative (^[Cc]lose[sd]? (#|...), etc.) is anchored at line-start only. Reproduced: pr_body ending in "...Did the thing.\n\nCloses #42 but leaves a follow-up needed for the migration script." has the entire last line dropped — including "but leaves a follow-up needed for the migration script" — not just the Closes #42 reference. No existing test exercises a closing-keyword line with trailing prose.
Suggestion: Anchor with $ after the reference (e.g. /^[Cc]lose[sd]? #[0-9]+$/ and the cross-repo equivalent), or only strip the line if nothing but whitespace remains after removing the matched reference. Add a test case for "closing keyword + trailing prose on the same line."
| if [ -n "${PR_BODY_FROM_RESULT}" ]; then | ||
| # Agent provided pr_body — strip trailing footer lines (Signed-off-by, | ||
| # GitHub auto-close keywords) so the script appends them once. | ||
| # Only strips from the end to preserve matching lines in body content. |
There was a problem hiding this comment.
[MEDIUM] pr_body stripping only removes trailing footer lines, unlike the hard, anywhere-in-body block enforced for the commit path
The legacy commit-body path strips Signed-off-by: lines unconditionally, anywhere in the body (sed '/^Signed-off-by:/d', line 480), and section 3b hard-blocks the entire push (exit 1) if a Signed-off-by: trailer appears anywhere in the commit body (lines 257-264). This new pr_body path only strips these patterns from the contiguous trailing block — a Signed-off-by: or Closes #N line anywhere else in pr_body survives untouched and is published verbatim, with no equivalent hard-block check applied to this new field.
Suggestion: Either strip these patterns globally from pr_body (matching legacy behavior) or add an explicit hard-fail check scanning the whole pr_body string for ^Signed-off-by:, at the same severity as the existing commit-body check.
| templates, use the one matching the issue context (e.g., `bug_report.md` | ||
| for bug fixes, `feature_request.md` for enhancements), or fall back to | ||
| the first/default template. If found, read it and note sections/prompts | ||
| — skip HTML comments (`<!-- ... -->`), extract only the visible headings |
There was a problem hiding this comment.
[MEDIUM] No guidance for a PR template that contains only HTML comments
The instruction to "skip HTML comments... extract only the visible headings and prompts" doesn't address the common case where the entire template is HTML comments with no visible headings at all (many repos use comment-only templates purely as instructions to humans). It's unclear whether the agent should treat this as "no template found" or attempt to synthesize headings from the comment text (which would contradict "skip HTML comments").
Suggestion: Add an explicit rule: if a template resolves to no visible sections after stripping comments, treat it the same as "no template found" and fall back to the best-effort description format.
| "42" "agent/42-widget" \ | ||
| "Fixes #42" "no" | ||
|
|
||
| run_pr_body_test "pr-body-resolves-keyword-stripped" \ |
There was a problem hiding this comment.
[MEDIUM] Missing test coverage for common closing-keyword variants the stripping regex doesn't match
The stripping regex requires a literal space between the keyword and #/owner/repo# (e.g. ^[Cc]lose[sd]? (#|...)). It doesn't match, and there's no test for, variants like Closes: #42 (colon-separated), closes(#42), or other plausible phrasings. If an agent writes one of these as the trailing line despite being told not to include closing references, it survives stripping and the final PR body ends up with a duplicate closing reference alongside the script's own appended Closes #N.
Suggestion: Add test cases for Closes: #N, closes(#N), and similar variants; broaden the regex if these are expected in practice, or explicitly document that only the tested patterns are defended against.
| end = NR | ||
| while (end > 0) { | ||
| l = lines[end] | ||
| if (l == "" || l ~ /^Signed-off-by:/ || l ~ /^[Cc]lose[sd]? (#|[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+#)/ || l ~ /^[Ff]ix(e[sd])? (#|[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+#)/ || l ~ /^[Rr]esolve[sd]? (#|[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+#)/) |
There was a problem hiding this comment.
[MEDIUM] Footer-stripping awk logic is hand-duplicated between production and test with no shared source
This build_pr_body() test helper re-implements the exact 15-line awk stripping block from scripts/post-code.sh:463-477 by hand rather than sourcing it. This mirrors a pre-existing pattern for rewrite_title/build_pr_body, but this PR compounds it with a second, more complex duplicated block — a future fix to the regex in one copy (e.g. the end-anchor bug flagged separately in this review) can silently diverge from the other while tests stay green, giving false confidence.
Suggestion: Extract the shared awk script into a small sourceable helper file/function that both post-code.sh and post-code-test.sh load, so tests actually exercise production code instead of a hand-copied duplicate.
| the first/default template. If found, read it and note sections/prompts | ||
| — skip HTML comments (`<!-- ... -->`), extract only the visible headings | ||
| and prompts. You will structure the `pr_body` field in the result file | ||
| to match template sections (see the structured output block below) — the post-script uses |
There was a problem hiding this comment.
[MEDIUM] Stale forward-reference — "the structured output block below" doesn't contain pr_body
This phrase points at the code block immediately following in step 3, which (by this PR's own design) only writes target_branch — pr_body is deliberately deferred to step 10a so a timeout never leaks placeholder text as the PR description. The "see below" pointer is factually wrong about where pr_body is actually written, and relies on the reader separately noticing the non-adjacent step 10a instruction. Looks like leftover phrasing from before the write was split into two steps.
Suggestion: Change to "You will structure the pr_body field later, in step 10a, to match these template sections" so the forward reference is accurate.
| @@ -294,7 +295,11 @@ Before planning, determine what kind of work this issue requires: | |||
| issue. | |||
| - **Label-gated** — if the issue has a label like `do-not-implement` or a gate | |||
There was a problem hiding this comment.
[MEDIUM, premature-decision] Template-to-issue-type matching (step 3 item 5) precedes the formal task-type classification here in step 6
Step 3 item 5 (line 169) asks the agent to select a template "matching the issue context (e.g., bug_report.md for bug fixes, feature_request.md for enhancements)" during repo-convention discovery — before this step, which is the procedure's own designated place for classifying bug vs. feature vs. test-only vs. already-fixed vs. label-gated. Template selection assumes a classification the process hasn't formally reached yet, with no cross-reference between the two steps — an unverified simplification shipped as final guidance.
Suggestion: Either move template selection after this step, or note in step 3 that it uses a preliminary/informal read of the issue that should be reconciled with the formal classification here if they conflict (e.g., a "bug_report" template picked early for an issue this step later classifies as "already-fixed" or "test-only").
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
LGTM. One minor note inline. I agree with @waynesun09's findings on the jq --arg safety claim and the maxLength headroom — thumbs-upped those.
| PR_BODY_FROM_RESULT="" | ||
| if [ -n "${RESULT_FILE}" ]; then | ||
| if ! PR_BODY_FROM_RESULT="$(jq -r '.pr_body // empty' "${RESULT_FILE}" 2>/dev/null)"; then | ||
| echo "::notice::pr_body not found in result file; using commit body" |
There was a problem hiding this comment.
[minor] Heads up — this fires on jq parse errors, not on a missing pr_body field. .pr_body // empty exits 0 with empty output when the field is absent, so that case falls through silently. Could trip someone up debugging why pr_body was ignored.
4 participants
Summary
pr_bodysupport from feat(code-agent): uses PR template if found fullsend#2979 to the agents repopr_bodyfield incode-result.jsonpr_bodyverbatim as PR description, falling back to commit body when absentprintfoverecho, tightenedsedpatterns,maxLengthconstraint,jq -nexamples, if/else clarityTest plan
post-code-test.shpasses all 62 tests including newpr_bodycasespr_bodyfield in CIMigrated from fullsend-ai/fullsend#2979. Closes fullsend-ai/fullsend#1575
🤖 Generated with Claude Code