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/)
|
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
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.
f828ec7 to
0c74ec6
Compare
|
🤖 Finished Review · ✅ Success · Started 7:00 AM UTC · Completed 7:08 AM UTC |
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>
0c74ec6 to
ee18489
Compare
|
🤖 Finished Review · ✅ Success · Started 7:17 AM UTC · Completed 7:33 AM UTC |
| 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.
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.
🤖 Generated with Claude Code