Skip to content

feat(code-agent): use PR template for pr_body in structured output #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rh-hemartin wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(code-agent): use PR template for pr_body in structured output #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions agents/code.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,11 @@ the review agent — if the triage was wrong, your code will fail review.
## Structured output

You MUST produce a JSON file at `$FULLSEND_OUTPUT_DIR/code-result.json`
that documents the target branch for PR creation. The `code-implementation`
skill describes the schema and the exact step where you write it. The
post-script reads this file to determine which branch to target the PR
against. Without this file, the validation loop rejects the run and retries.
with `target_branch` (required) and optionally `pr_body` for the PR
description. The `code-implementation` skill describes the schema and
the exact steps where you write each field. The post-script reads this
file to determine the PR target branch and description. Without this
file, the validation loop rejects the run and retries.

After writing the file, validate it before exiting:

Expand Down
7 changes: 6 additions & 1 deletion schemas/code-result.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "code-result.schema.json",
"title": "Code Agent Result",
"description": "Structured output from the code agent documenting the target branch for PR creation.",
"description": "Structured output from the code agent documenting the target branch and PR body for PR creation.",
"type": "object",
"required": ["target_branch"],
"additionalProperties": false,
Expand All @@ -11,6 +11,11 @@
"type": "string",
"description": "Branch the PR should target. Determined by the agent from issue context.",
"pattern": "^[a-zA-Z0-9._/-]+$"
},
"pr_body": {
"type": "string",
"maxLength": 65536,

@waynesun09 waynesun09 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

"description": "PR description used as PR body instead of commit body. Supports markdown formatting and template-compliant structure without gitlint line-length constraints."
}
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
}
}
153 changes: 148 additions & 5 deletions scripts/post-code-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,35 @@ build_pr_body() {
local issue_number="$2"
local branch="$3"
local scan_range="$4"
local pr_body_from_result="${5:-}" # optional: agent-provided pr_body

local description=""
if [ -n "${pr_body_from_result}" ]; then
Comment thread
rh-hemartin marked this conversation as resolved.
# Agent provided pr_body — strip trailing footer lines only.
description="$(printf '%s\n' "${pr_body_from_result}" | awk '
{ lines[NR] = $0 }
END {
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_.-]+#)/)

@waynesun09 waynesun09 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

end--
else
break
}
for (i = 1; i <= end; i++)
print lines[i]
}
')"
fi

local description
if [ -z "${commit_body}" ]; then
description="Automated implementation for issue #${issue_number}."
else
description="${commit_body}"
# Fall back if pr_body was absent or stripped to empty
if [ -z "${description}" ]; then
if [ -z "${commit_body}" ]; then
description="Automated implementation for issue #${issue_number}."
else
description="${commit_body}"
fi
fi

echo "${description}
Expand Down Expand Up @@ -250,6 +273,126 @@ count_closes_test "single-closes-with-body" \
count_closes_test "single-closes-empty-body" \
"" "99"

# Verify pr_body path strips Closes lines (agent may include them)
count_closes_pr_body_test() {
local test_name="$1"
local pr_body="$2"
local issue_number="$3"

local actual
actual="$(build_pr_body "" "${issue_number}" "agent/${issue_number}-fix" "abc123..def456" "${pr_body}")"

local count
count=$(echo "${actual}" | grep -c "Closes #${issue_number}" || true)

if [ "${count}" -ne 1 ]; then
echo "FAIL: ${test_name}"
echo " expected exactly 1 'Closes #${issue_number}', found ${count}"
echo " in body:"
echo "${actual}" | sed 's/^/ /'
FAILURES=$((FAILURES + 1))
return
fi

echo "PASS: ${test_name}"
}

count_closes_pr_body_test "single-closes-pr-body-with-closes" \
Comment thread
rh-hemartin marked this conversation as resolved.
"## Summary

Implemented widget rendering.

Closes #42" "42"

count_closes_pr_body_test "single-closes-pr-body-with-cross-repo-closes" \
"## Summary

Implemented widget rendering.

Closes fullsend-ai/agents#42" "42"

# --- pr_body path test cases ---

# Helper for pr_body tests (fifth arg is pr_body from result file)
run_pr_body_test() {
local test_name="$1"
local pr_body="$2"
local issue_number="$3"
local branch="$4"
local check_pattern="$5"
local expect_present="$6" # "yes" or "no"

local actual
actual="$(build_pr_body "" "${issue_number}" "${branch}" "abc123..def456" "${pr_body}")"

if [ "${expect_present}" = "yes" ]; then
if ! echo "${actual}" | grep -qF "${check_pattern}"; then
echo "FAIL: ${test_name}"
echo " expected to find: '${check_pattern}'"
echo " in body:"
echo "${actual}" | sed 's/^/ /'
FAILURES=$((FAILURES + 1))
return
fi
else
if echo "${actual}" | grep -qF "${check_pattern}"; then
echo "FAIL: ${test_name}"
echo " expected NOT to find: '${check_pattern}'"
echo " in body:"
echo "${actual}" | sed 's/^/ /'
FAILURES=$((FAILURES + 1))
return
fi
fi

echo "PASS: ${test_name}"
}

# pr_body provided by agent should appear in final PR body
run_pr_body_test "pr-body-from-result" \
$'## Summary\n\nAdded widget rendering.\n\n## Testing\n\nManual test.' \
"42" "agent/42-widget" \
"Added widget rendering." "yes"

# pr_body should NOT be word-wrapped (it's verbatim)
run_pr_body_test "pr-body-verbatim" \
$'## Summary\n\nThis is a very long line that would normally be word-wrapped by the legacy commit-body awk logic but should remain intact when coming from pr_body.' \
"42" "agent/42-widget" \
"This is a very long line that would normally be word-wrapped by the legacy commit-body awk logic but should remain intact when coming from pr_body." "yes"

# pr_body that strips to empty should fall back to automated description
run_pr_body_test "pr-body-strips-to-empty" \
$'Closes #42' \
"42" "agent/42-widget" \
"Automated implementation for issue #42." "yes"

# Cross-repo Closes in trailing footer should be stripped
run_pr_body_test "pr-body-cross-repo-closes" \
$'## Summary\n\nImplemented widget rendering.\n\nCloses fullsend-ai/agents#42' \
"42" "agent/42-widget" \
"Closes fullsend-ai/agents#42" "no"

# Closes-like line in body content (not footer) should be preserved
run_pr_body_test "pr-body-closes-in-content-preserved" \
$'## Summary\n\nThis fixes the issue where Closes #99 was not handled.\n\n## Testing\n\nManual test.' \
"42" "agent/42-widget" \
"Closes #99 was not handled" "yes"

# Multiple trailing blank lines before footer should all be stripped
count_closes_pr_body_test "pr-body-trailing-blanks-before-footer" \
$'## Summary\n\nDid the thing.\n\n\n\nCloses #42' "42"

# GitHub auto-close keyword variants should be stripped from footer
run_pr_body_test "pr-body-fixes-keyword-stripped" \
$'## Summary\n\nFixed the bug.\n\nFixes #42' \
"42" "agent/42-widget" \
"Fixes #42" "no"

run_pr_body_test "pr-body-resolves-keyword-stripped" \

@waynesun09 waynesun09 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

$'## Summary\n\nResolved the issue.\n\nResolves #42' \
"42" "agent/42-widget" \
"Resolves #42" "no"

# ---------------------------------------------------------------------------
# Test helper — reimplements the no-op detection logic from post-code.sh
# so we can test it without a git repo or network access.
Expand Down
49 changes: 40 additions & 9 deletions scripts/post-code.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,15 +446,46 @@ fi
echo "Creating PR..."

COMMIT_SUBJECT="$(git log -1 --format='%s' HEAD)"
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 }
{ buf = (buf ? buf " " $0 : $0) }
END { if (buf) print buf }
')"

# Read pr_body from agent output. Fall back to commit body if absent.
PR_BODY_FROM_RESULT=""

@waynesun09 waynesun09 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

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"

@ralphbean ralphbean Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

PR_BODY_FROM_RESULT=""
fi
fi

if [ -n "${PR_BODY_FROM_RESULT}" ]; then

@waynesun09 waynesun09 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] When pr_body strips to empty, the real commit body is discarded — falls straight to the generic placeholder even if the commit body is substantive

In the legacy (no-pr_body) path, COMMIT_BODY is always computed from git log -1 --format='%b' HEAD. In this new path, once PR_BODY_FROM_RESULT is non-empty, the else branch that computes the real commit body (line 480) is skipped entirely — even if pr_body strips down to nothing (e.g. it only contained a Closes #N line). The final description falls to "Automated implementation for issue #N." even though a perfectly good commit body may exist. No test exercises this with a non-empty commit_body argument alongside a pr_body that strips to empty.

Suggestion: When pr_body strips to empty, fall back to the real commit body (compute it unconditionally or lazily) before falling back to the generic placeholder, and add a test case covering this.

# 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.

@waynesun09 waynesun09 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

COMMIT_BODY="$(printf '%s\n' "${PR_BODY_FROM_RESULT}" | awk '
{ lines[NR] = $0 }
END {
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_.-]+#)/)

@waynesun09 waynesun09 Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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."

end--
else
break
}
for (i = 1; i <= end; i++)
print lines[i]
}
')"
else
Comment thread
rh-hemartin marked this conversation as resolved.
Comment thread
rh-hemartin marked this conversation as resolved.
# Fall back to unwrapped commit body (legacy path)
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 }
{ buf = (buf ? buf " " $0 : $0) }
END { if (buf) print buf }
')"
fi

# ---------------------------------------------------------------------------
# Ensure PR title includes an issue reference.
Expand Down
Loading
Loading