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 runReviewFindingsMedium
Low
Prior review resolution
Previous run (2)ReviewFindingsMedium
Low
Prior review resolution
Previous run (3)ReviewVerdict: 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 (4)
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
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}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
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.
rh-hemartin
force-pushed
the
feat/pr-body
branch
from
ee18489 to
b82bdce
Compare
|
🤖 Review · |
rh-hemartin
force-pushed
the
feat/pr-body
branch
from
b82bdce to
bdafc8a
Compare
|
🤖 Finished Review · ✅ Success · Started 6:52 AM UTC · Completed 7:13 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
Add pr_body to code-result schema so agents can write PR descriptions matching repo templates. Post-script reads, secret-scans, and sanitizes pr_body before creating the PR. 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
bdafc8a to
a108cb8
Compare
|
🤖 Finished Review · ✅ Success · Started 7:43 AM UTC · Completed 8:00 AM UTC |
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, Grok). Posted 4 inline comments for the unique medium findings not already covered by prior review rounds.
All prior CRITICAL/HIGH findings verified as resolved (jq --arg injection fixed, maxLength reduced to 60000, pr_body secret scan added, footer regex end-anchored). Two approvals (ascerra, ralphbean) remain valid.
Assisted-by: Claude (review), Grok (review)
| expansion, then pass it to `jq --arg`: | ||
|
|
||
| ```bash | ||
| read -r -d '' pr_body <<'PRBODY' |
There was a problem hiding this comment.
[MEDIUM] read -r -d '' returns exit 1, breaks under set -e
read -r -d '' reads until a null byte. Heredocs never contain null bytes, so read hits EOF and returns exit code 1 — even though $pr_body is correctly populated. If an agent wraps this in a script with set -e or set -euo pipefail, the shell terminates before the jq line executes and pr_body is never written to the result file.
Without set -e it works (the non-zero exit doesn't prevent the next command from running), but the pattern is fragile and will silently break if the execution environment changes.
Suggestion: Append || true:
read -r -d '' pr_body <<'PRBODY' || true
<pr_body content>
PRBODYOr replace with cat command substitution (returns 0, also strips trailing newlines which is generally desirable for PR body content):
pr_body=$(cat <<'PRBODY'
<pr_body content>
PRBODY
)§ Consensus — flagged independently by 3/3 review agents.
|
|
||
| - [x] Branch is not main/master (\`${BRANCH}\`) | ||
| - [x] Secret scan passed (gitleaks — \`${SCAN_RANGE}\`) | ||
| - [x] PR body secret scan passed (gitleaks — no-git) |
There was a problem hiding this comment.
[MEDIUM] Unconditional "PR body secret scan passed" checklist item is inaccurate
This line is hardcoded and always rendered, regardless of what actually happened. Three scenarios produce misleading audit claims:
- No
pr_bodyexists (legacy commit-body path) — no scan ran, but checklist says it passed - gitleaks detected a secret — script correctly fell back to commit body, but checklist still says "passed"
- gitleaks itself errored (corrupt binary, permission denied) — same misleading "passed" message
The security behavior is correct in all cases (no secrets leak), but the audit trail is inaccurate. The same unconditional line appears in the test helper at post-code-test.sh:176.
Suggestion: Make the line conditional on what actually happened:
if [ -n "${PR_BODY_SCANNED:-}" ]; then
PR_BODY_SCAN_LINE="- [x] PR body secret scan passed (gitleaks — no-git)"
else
PR_BODY_SCAN_LINE="- [x] PR body secret scan: N/A (commit body path)"
fiSet PR_BODY_SCANNED=1 after a successful gitleaks run. For the "blocked" case, use "- [x] PR body secret scan: blocked, fell back to commit body".
§ Consensus — flagged independently by 2/3 review agents.
|
|
||
| if [ -z "${COMMIT_BODY}" ]; then | ||
| # pr_body stripped to empty — fall back to commit body before placeholder | ||
| COMMIT_BODY_RAW="$(git log -1 --format='%b' HEAD | sed '/^Signed-off-by:/d' | sed '/^Closes #/d' | sed -e :a -e '/^\n*$/{ $d; N; ba; }')" | ||
| COMMIT_BODY="$(echo "${COMMIT_BODY_RAW}" | awk ' | ||
| /^$/ { if (buf) print buf; print; buf=""; next } | ||
| /^[-*#>]|^ / { if (buf) print buf; buf=""; print; next } | ||
| /^Closes / { if (buf) print buf; buf=""; print; next } |
There was a problem hiding this comment.
[MEDIUM] Intra-file duplication of commit-body extraction pipeline
This 7-line git log | sed | sed | sed + awk pipeline is identical to the one at lines 521–529 (the legacy else branch). A future change to the extraction logic must be applied to both copies, and divergence between them would be silent.
The prior review flagged cross-file duplication (test vs production) — author noted "multi-scripts not supported." This is intra-file duplication within the same script, fixable with a local function:
extract_commit_body() {
local raw
raw="$(git log -1 --format='%b' HEAD | sed '/^Signed-off-by:/d' | sed '/^Closes #/d' | sed -e :a -e '/^\n*$/{ $d; N; ba; }')"
echo "${raw}" | awk '
/^$/ { if (buf) print buf; print; buf=""; next }
/^[-*#>]|^ / { if (buf) print buf; buf=""; print; next }
/^Closes / { if (buf) print buf; buf=""; print; next }
{ buf = (buf ? buf " " $0 : $0) }
END { if (buf) print buf }
'
}Then both call sites become COMMIT_BODY="$(extract_commit_body)".
§ Consensus — flagged independently by 2/3 review agents.
| if [ -n "${PR_BODY_FROM_RESULT}" ]; then | ||
| PR_BODY_TMP="$(mktemp)" | ||
| printf '%s\n' "${PR_BODY_FROM_RESULT}" > "${PR_BODY_TMP}" | ||
| if ! gitleaks detect --source "${PR_BODY_TMP}" --no-git --redact 2>/dev/null; then |
There was a problem hiding this comment.
[MEDIUM] gitleaks stderr suppressed — runtime errors misdiagnosed as "secret detected"
2>/dev/null discards all stderr. If gitleaks fails due to a config error, missing binary, permission denied, or corrupt installation, the non-zero exit code triggers "BLOCKED — secret detected in pr_body" when no secret was actually found. The conservative fallback (using commit body) is correct, but the diagnostic misleads anyone debugging a workflow failure — they'll chase a phantom secret instead of a broken tool.
Suggestion: Capture the exit code and distinguish detection from error:
gitleaks detect --source "${PR_BODY_TMP}" --no-git --redact 2>/dev/null
GL_RC=$?
if [ "${GL_RC}" -eq 1 ]; then
echo "::warning::BLOCKED — secret detected in pr_body; falling back to commit body"
PR_BODY_FROM_RESULT=""
elif [ "${GL_RC}" -ne 0 ]; then
echo "::warning::gitleaks scan failed (exit ${GL_RC}); falling back to commit body"
PR_BODY_FROM_RESULT=""
fiOr at minimum, redirect stderr to ::debug:: instead of /dev/null so CI logs retain the diagnostic when needed.
§ Consensus — flagged independently by 2/3 review agents.
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