Skip to content

fix(#1824): add body to enrollment commit messages #1825

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

Merged
ifireball merged 4 commits into from
Jun 4, 2026
Merged

fix(#1824): add body to enrollment commit messages #1825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d7451c1
fix(#1824): add body to enrollment commit messages
Jun 3, 2026
2dea270
fix: exercise all commit-message paths in reconcile-repos test
Jun 3, 2026
5d648af
fix: address review feedback on PR #1825
Jun 3, 2026
d0de400
fix: address review feedback on PR #1825
Jun 3, 2026
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
196 changes: 171 additions & 25 deletions internal/scaffold/fullsend-repo/scripts/reconcile-repos-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,20 @@ trap 'rm -rf "${TMPDIR}"' EXIT
CONFIG_DIR="${TMPDIR}/config"
MOCK_BIN="${TMPDIR}/bin"
GH_LOG="${TMPDIR}/gh-calls.log"
COMMIT_MSGS_LOG="${TMPDIR}/commit-msgs.log"
mkdir -p "${CONFIG_DIR}/templates" "${MOCK_BIN}"

cat > "${CONFIG_DIR}/config.yaml" <<'EOF'
version: 1
repos:
test-repo:
enabled: true
new-repo:
enabled: true
refresh-repo:
enabled: true
removed-repo:
enabled: false
EOF

cat > "${CONFIG_DIR}/templates/shim-workflow-call.yaml" <<'EOF'
Expand All @@ -41,9 +48,9 @@ cat > "${MOCK_BIN}/yq" <<'EOF'
#!/usr/bin/env bash
query="${1:-}"
if [[ "$query" == *"enabled == true"* ]]; then
echo "test-repo"
printf '%s\n' "test-repo" "new-repo" "refresh-repo"
elif [[ "$query" == *"enabled == false"* ]]; then
:
echo "removed-repo"
else
echo "unexpected yq query: $*" >&2
exit 1
Expand All @@ -60,12 +67,39 @@ for arg in "\$@"; do
done
printf '\n' >> "${GH_LOG}"

if [[ "\$1" == "pr" && "\$2" == "list" ]]; then
for arg in "\$@"; do
if [[ "\$arg" == "fullsend/onboard" ]]; then
echo "https://github.com/test-org/test-repo/pull/18"
fi
done
# Handle pr subcommands.
if [[ "\$1" == "pr" ]]; then
case "\$2" in
list)
# Parse --repo and --head to differentiate responses.
repo_arg=""
head_arg=""
prev=""
for arg in "\$@"; do
case "\$prev" in
--repo) repo_arg="\$arg" ;;
--head) head_arg="\$arg" ;;
esac
prev="\$arg"
done
if [[ "\$head_arg" == "fullsend/onboard" ]]; then
case "\$repo_arg" in
test-org/test-repo)
echo "https://github.com/test-org/test-repo/pull/18" ;;
test-org/refresh-repo)
echo "https://github.com/test-org/refresh-repo/pull/5" ;;
esac
fi
exit 0
;;
create)
echo "https://github.com/test-org/mock/pull/99"
exit 0
;;
close)
exit 0
;;
esac
exit 0
fi

Expand All @@ -74,57 +108,92 @@ if [[ "\$1" != "api" ]]; then
exit 1
fi

# Extract --jq filter if present.
# Extract flags from the gh api call.
jq_filter=""
has_input=false
method="GET"
field_message=""
shift # consume "api"
endpoint="\$1"; shift
while [[ \$# -gt 0 ]]; do
case "\$1" in
--jq) jq_filter="\$2"; shift 2 ;;
--input) shift 2 ;; # consume --input -
--method|--field) shift 2 ;;
--input) has_input=true; shift 2 ;; # consume --input -
--method) method="\$2"; shift 2 ;;
--field)
if [[ "\$2" == message=* ]]; then
field_message="\${2#message=}"
fi
shift 2
;;
--silent) shift ;;
*) shift ;;
esac
done

# Capture commit messages from stdin for the git/commits endpoint.
input_data=""
if [[ "\$has_input" == "true" ]]; then
input_data=\$(cat)
if [[ "\$endpoint" == */git/commits ]]; then
printf '%s\0' "\$input_data" >> "${COMMIT_MSGS_LOG}"
fi
fi

json=""
rc=0
case "\$endpoint" in
repos/test-org/test-repo/actions/variables/*)
# Variable not found — 404.
repos/test-org/*/actions/variables/*)
# Variable not found — 404 for all test repos.
json='{"status":"404","message":"Not Found"}'
rc=1
;;
repos/test-org/test-repo/contents/.github/workflows/fullsend.yaml)
repos/test-org/test-repo/contents/*)
# test-repo: stale shim exists on default branch.
json='{"content":"c3RhbGUgc2hpbSB0ZW1wbGF0ZQo=","sha":"file-sha"}'
;;
repos/test-org/test-repo)
json='{"default_branch":"main","private":false}'
repos/test-org/removed-repo/contents/*)
if [[ "\$method" == "DELETE" ]]; then
# Capture the removal commit message for validation.
if [[ -n "\$field_message" ]]; then
removal_json=\$(jq -n --arg msg "\$field_message" '{message: \$msg}')
printf '%s\0' "\$removal_json" >> "${COMMIT_MSGS_LOG}"
fi
else
# Shim exists — return content and SHA for GET requests.
json='{"content":"c3RhbGUgc2hpbSB0ZW1wbGF0ZQo=","sha":"remove-file-sha"}'
fi
;;
repos/test-org/test-repo/git/ref/heads/main)
repos/test-org/*/contents/*)
# new-repo, refresh-repo: no shim on default branch.
rc=1
;;
repos/test-org/*/git/ref/heads/*)
json='{"object":{"sha":"base-sha"}}'
;;
repos/test-org/test-repo/git/commits/base-sha)
repos/test-org/*/git/commits/base-sha)
json='{"tree":{"sha":"base-tree-sha"}}'
;;
repos/test-org/test-repo/git/blobs)
repos/test-org/*/git/blobs)
json='{"sha":"blob-sha"}'
;;
repos/test-org/test-repo/git/trees)
repos/test-org/*/git/trees)
json='{"sha":"tree-sha"}'
;;
repos/test-org/test-repo/git/commits)
repos/test-org/*/git/commits)
json='{"sha":"desired-commit-sha"}'
;;
repos/test-org/test-repo/git/refs)
repos/test-org/*/git/refs)
# Branch creation — fail so the script falls back to PATCH.
rc=1
;;
repos/test-org/test-repo/git/refs/heads/fullsend/onboard)
repos/test-org/*/git/refs/heads/*)
# Branch update or delete — always succeed.
rc=0
;;
repos/test-org/test-repo/git/refs/heads/fullsend/offboard)
rc=0
repos/test-org/*)
# Repo metadata (default branch, visibility).
json='{"default_branch":"main","private":false}'
;;
*)
echo "unexpected gh api endpoint: \$endpoint" >&2
Expand Down Expand Up @@ -169,3 +238,80 @@ if grep -q "contents/.github/workflows/fullsend.yaml.*--method PUT" "${GH_LOG}";
fi

@fullsend-ai-review fullsend-ai-review Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] test-inadequate

Only the stale-shim-update commit message path is exercised by the test. The enrollment, refresh-existing-PR, and removal paths are not reached, so their commit messages are not validated.

Suggested fix: Add test scenarios exercising the other three commit message paths: a repo with no existing shim (enrollment), a repo with an existing enrollment PR (refresh), and a disabled repo with an existing shim (removal).

@fullsend-ai-review fullsend-ai-review Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] test-inadequate

The removal commit message is sent via the Contents API DELETE endpoint using --field, not via the Git Objects API with --input stdin. The test infrastructure only captures stdin payloads for git/commits calls, so the removal message cannot be validated even if a removal scenario were added.

Suggested fix: Extend the mock gh script to also capture --field message= values for DELETE calls to the contents endpoint.


echo "PASS: stale shim branch update is atomic"

# ===========================
# Test: commit messages include a non-trivial body
# ===========================

if [ ! -f "${COMMIT_MSGS_LOG}" ]; then
echo "FAIL: no commit messages were captured"
exit 1
fi

# The log contains null-delimited JSON payloads from git/commits calls
# and Contents API DELETE calls (removal path).
# Extract each message and verify it has a subject, blank line, and body.
msg_index=0
fail=0
while IFS= read -r -d '' json_payload; do
[ -z "$json_payload" ] && continue
msg=$(printf '%s' "$json_payload" | jq -r '.message')
msg_index=$((msg_index + 1))

# A well-formed message has: subject, blank line, body.
subject=$(printf '%s\n' "$msg" | head -n1)
second_line=$(printf '%s\n' "$msg" | sed -n '2p')
body=$(printf '%s\n' "$msg" | tail -n +3)

if [ -n "$second_line" ]; then
echo "FAIL: commit message #${msg_index} missing blank line after subject"
echo " subject: $subject"
echo " line 2: $second_line"
fail=1
continue
fi

body_trimmed=$(printf '%s' "$body" | tr -d '[:space:]')
if [ -z "$body_trimmed" ]; then
echo "FAIL: commit message #${msg_index} has no body"
echo " subject: $subject"
fail=1
continue
fi

# Verify subject does not exceed 50 characters (conventional commit guideline).
if [ "${#subject}" -gt 50 ]; then
echo "FAIL: commit message #${msg_index} subject exceeds 50 chars"
echo " subject (${#subject} chars): $subject"
fail=1
fi

# Verify no line in the message exceeds 72 characters.

@fullsend-ai-review fullsend-ai-review Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] test-integrity

The commit message line-length check applies the 72-character limit to all lines including the subject. Conventional commit convention limits subjects to 50 characters. Not a bug (the actual subjects are well under 50 chars), but the test would not catch a subject between 51-72 characters if one were introduced later.

while IFS= read -r bline; do
if [ "${#bline}" -gt 72 ]; then
echo "FAIL: commit message #${msg_index} has a line exceeding 72 chars"
echo " line (${#bline} chars): $bline"

@fullsend-ai-review fullsend-ai-review Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] test-inadequate

The commit message validation loop checks that each captured message is well-formed but does not assert the expected count of messages (should be 4: one each for update, add, refresh, and remove). If a code path silently fails to produce a commit, the test would still pass as long as at least one message is captured and well-formed.

Suggested fix: Add an assertion after the validation loop: [ "$msg_index" -eq 4 ] || { echo "FAIL: expected 4 commit messages, got $msg_index"; exit 1; }

fail=1
fi
done <<< "$msg"
done < "${COMMIT_MSGS_LOG}"

if [ "$msg_index" -eq 0 ]; then
echo "FAIL: no commit messages found in log"
exit 1
fi

# Expect exactly 4 commit messages: update (stale shim), refresh (existing PR),
# add (new enrollment), and remove (unenrollment).
if [ "$msg_index" -ne 4 ]; then
echo "FAIL: expected 4 commit messages but found $msg_index"
exit 1
fi

if [ "$fail" -ne 0 ]; then
echo "--- captured commit messages ---"
cat "${COMMIT_MSGS_LOG}"
exit 1
fi

echo "PASS: commit messages include a non-trivial body (≤72 chars/line)"
23 changes: 19 additions & 4 deletions internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,21 @@ UPDATE_PR_BODY="This PR updates the fullsend shim workflow to match the current

The shim content has drifted from the template — this brings it back in sync."

UPDATE_COMMIT_MSG="chore: update fullsend shim workflow

Update the shim workflow to match the current template
in the .fullsend config repo."

ENROLL_COMMIT_MSG="chore: add fullsend shim workflow

Add the shim workflow that routes repository events to
the fullsend agent dispatch pipeline."

UNENROLL_COMMIT_MSG="chore: remove fullsend shim workflow

Remove the shim workflow. The repo has been set to
enabled: false in the fullsend config."

if [ ! -f "$SHIM_TEMPLATE" ]; then
echo "::error::shim template not found at $SHIM_TEMPLATE"
exit 1
Expand Down Expand Up @@ -312,7 +327,7 @@ if [ -n "$ENABLED_REPOS" ]; then
# Shim is stale — update via PR to respect branch protection.
echo "⟳ $REPO enrolled but shim is stale — creating update PR"

if ! write_shim_to_branch_from_default "$REPO" "$ENROLL_BRANCH" "$EXPECTED_B64" "chore: update fullsend shim workflow"; then
if ! write_shim_to_branch_from_default "$REPO" "$ENROLL_BRANCH" "$EXPECTED_B64" "$UPDATE_COMMIT_MSG"; then
FAILED=$((FAILED + 1))
continue
fi
Expand Down Expand Up @@ -344,7 +359,7 @@ if [ -n "$ENABLED_REPOS" ]; then
if [ -n "$EXISTING_PR" ]; then
echo "✓ $REPO has existing enrollment PR: $EXISTING_PR"
# Update the shim on the existing branch to reflect the latest content.
if ! write_shim_to_branch_from_default "$REPO" "$ENROLL_BRANCH" "$(shim_content_b64)" "chore: update fullsend shim workflow"; then
if ! write_shim_to_branch_from_default "$REPO" "$ENROLL_BRANCH" "$(shim_content_b64)" "$UPDATE_COMMIT_MSG"; then
FAILED=$((FAILED + 1))
else
ENROLLED=$((ENROLLED + 1))
Expand All @@ -362,7 +377,7 @@ if [ -n "$ENABLED_REPOS" ]; then
continue

@fullsend-ai-review fullsend-ai-review Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] code-organization

ENROLL_COMMIT_MSG and UNENROLL_COMMIT_MSG are defined inside their respective loop bodies (reassigned on every iteration), while UPDATE_COMMIT_MSG is defined once at the top level alongside UPDATE_PR_BODY. All three are constant strings — hoisting them to the top would be more consistent.

Suggested fix: Move ENROLL_COMMIT_MSG and UNENROLL_COMMIT_MSG definitions to the top of the script alongside UPDATE_COMMIT_MSG (after line 51).

fi

if ! write_shim_to_branch_from_default "$REPO" "$ENROLL_BRANCH" "$SHIM_CONTENT" "chore: add fullsend shim workflow"; then
if ! write_shim_to_branch_from_default "$REPO" "$ENROLL_BRANCH" "$SHIM_CONTENT" "$ENROLL_COMMIT_MSG"; then
FAILED=$((FAILED + 1))
continue
fi
Expand Down Expand Up @@ -446,7 +461,7 @@ if [ -n "$DISABLED_REPOS" ]; then
# Delete the shim workflow on the removal branch.
if ! gh api "repos/$ORG/$REPO/contents/$SHIM_PATH" \
--method DELETE \
--field "message=chore: remove fullsend shim workflow" \
--field "message=$UNENROLL_COMMIT_MSG" \
--field "branch=$UNENROLL_BRANCH" \
--field "sha=$FILE_SHA" \
--silent; then
Expand Down
Loading