Skip to content

Commit e5a9ba2

Browse files
sylvansysclaude
sylvansys
and
claude
authored
fix: Quick fixes for reviewer comments (#58)
* fix: Quick fixes for reviewer comments Phase 1 fixes from issue #56: - Use ⏭️ emoji for skipped status (instead of ⚠️) - Fix 404 agent prompt links by using SHA instead of branch name - Remove split test - use emoji format for all reviewers - Update intro line: "PR feedback updates from your reviewer agents..." - Add CI failure (exit 1) when checks fail Closes #56 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: Ensure comment posts before CI fails Add `if: always()` to post-comment steps so review results are posted even when the reviewer action finds failures. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: Remove DEBUG statements from requirements reviewer Addresses YAGNI feedback from code-quality reviewer. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * refactor: Extract shared JSON result parsing logic Address DRY violation by creating shared extract-result.sh script. Both reviewers now use the same extraction logic, reducing duplication from ~90 lines to ~3 lines per reviewer. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * refactor: Extract skip result generation to shared script Further address DRY violation by adding generate_skipped_result function. Both reviewers now use the shared function for skip handling. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * chore: Defer CI failure feature to follow-up Remove exit 1 logic - the feature works correctly but catches pre-existing code quality issues in the codebase that need to be addressed separately before this can be enabled. The other Phase 1 fixes (emojis, links, intro text) are ready. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent acbd1e0 commit e5a9ba2

4 files changed

Lines changed: 115 additions & 136 deletions

File tree

‎.github/actions/code-quality-reviewer/action.yml‎

Lines changed: 11 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -129,67 +129,22 @@ runs:
129129
id: extract
130130
shell: bash
131131
run: |
132+
# Use shared extraction logic
133+
source "${{ github.workspace }}/.github/actions/shared/extract-result.sh"
134+
132135
# Handle skipped case first
133136
if [ "${{ steps.validate.outputs.skip }}" = "true" ]; then
134-
REASON="${{ steps.validate.outputs.skip_reason }}"
135-
echo "passed=true" >> $GITHUB_OUTPUT
136-
echo "checks_passed=0" >> $GITHUB_OUTPUT
137-
echo "checks_failed=0" >> $GITHUB_OUTPUT
138-
echo "checks_skipped=4" >> $GITHUB_OUTPUT
139-
RESULT=$(jq -n --arg reason "$REASON" '{
140-
checks: [
141-
{name: "DRY Compliance", status: "skipped", result: $reason, reasoning: $reason, files: []},
142-
{name: "YAGNI Compliance", status: "skipped", result: $reason, reasoning: $reason, files: []},
143-
{name: "Modularity", status: "skipped", result: $reason, reasoning: $reason, files: []},
144-
{name: "Maintainability", status: "skipped", result: $reason, reasoning: $reason, files: []}
145-
],
146-
message: $reason
147-
}')
148-
echo "result=$(echo "$RESULT" | jq -c '.')" >> $GITHUB_OUTPUT
137+
generate_skipped_result "${{ steps.validate.outputs.skip_reason }}" \
138+
"DRY Compliance" "YAGNI Compliance" "Modularity" "Maintainability"
139+
echo "passed=$PASSED" >> $GITHUB_OUTPUT
140+
echo "result=$RESULT" >> $GITHUB_OUTPUT
141+
echo "checks_passed=$CHECKS_PASSED" >> $GITHUB_OUTPUT
142+
echo "checks_failed=$CHECKS_FAILED" >> $GITHUB_OUTPUT
143+
echo "checks_skipped=$CHECKS_SKIPPED" >> $GITHUB_OUTPUT
149144
exit 0
150145
fi
151146
152-
# Initialize defaults
153-
CHECKS_PASSED=0
154-
CHECKS_FAILED=0
155-
CHECKS_SKIPPED=0
156-
PASSED="false"
157-
RESULT='{"checks":[],"message":"Review failed to complete"}'
158-
159-
EXEC="${{ steps.review.outputs.execution_file }}"
160-
if [ -f "$EXEC" ]; then
161-
# Get ALL text from assistant messages, joined together
162-
ALL_TEXT=$(jq -r '[.[] | select(.type=="assistant") | .message.content[]? | select(.type=="text") | .text] | join("\n")' "$EXEC" 2>/dev/null || echo "")
163-
# Strategy 1: Extract from ```json block
164-
JSON=$(echo "$ALL_TEXT" | sed -n '/```json/,/```/{/```/d;p;}' | head -100)
165-
166-
# Strategy 2: If no json block, try to find raw JSON object with "checks" key
167-
if ! echo "$JSON" | jq . >/dev/null 2>&1; then
168-
JSON=$(echo "$ALL_TEXT" | grep -oP '\{"checks":\s*\[.*\]\s*(,"message":[^}]*)?\}' | head -1)
169-
fi
170-
171-
# Strategy 3: Extract any JSON object starting with {"checks"
172-
if ! echo "$JSON" | jq . >/dev/null 2>&1; then
173-
JSON=$(echo "$ALL_TEXT" | perl -ne 'print if /\{"checks":\[/.../"message":/' | tr '\n' ' ')
174-
fi
175-
176-
if echo "$JSON" | jq . >/dev/null 2>&1; then
177-
# Count checks by status
178-
CHECKS_PASSED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "passed")] | length')
179-
CHECKS_FAILED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "failed")] | length')
180-
CHECKS_SKIPPED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "skipped")] | length')
181-
182-
# Only pass if ALL checks passed (no failures)
183-
if [ "$CHECKS_FAILED" = "0" ] && [ "$CHECKS_PASSED" -gt "0" ]; then
184-
PASSED="true"
185-
elif [ "$CHECKS_FAILED" = "0" ] && [ "$CHECKS_PASSED" = "0" ] && [ "$CHECKS_SKIPPED" -gt "0" ]; then
186-
# All skipped is considered a pass
187-
PASSED="true"
188-
fi
189-
190-
RESULT=$(echo "$JSON" | jq -c '.')
191-
fi
192-
fi
147+
extract_review_result "${{ steps.review.outputs.execution_file }}"
193148
194149
# Always output
195150
echo "passed=$PASSED" >> $GITHUB_OUTPUT

‎.github/actions/requirements-reviewer/action.yml‎

Lines changed: 11 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -168,80 +168,23 @@ runs:
168168
id: extract
169169
shell: bash
170170
run: |
171+
# Use shared extraction logic
172+
source "${{ github.workspace }}/.github/actions/shared/extract-result.sh"
173+
171174
# Handle skipped case first
172175
if [ "${{ steps.validate.outputs.skip }}" = "true" ]; then
173-
REASON="${{ steps.validate.outputs.skip_reason }}"
174-
echo "passed=true" >> $GITHUB_OUTPUT
176+
generate_skipped_result "${{ steps.validate.outputs.skip_reason }}" \
177+
"Completeness" "Scope" "Traceability"
178+
echo "passed=$PASSED" >> $GITHUB_OUTPUT
175179
echo "skipped=true" >> $GITHUB_OUTPUT
176-
echo "checks_passed=0" >> $GITHUB_OUTPUT
177-
echo "checks_failed=0" >> $GITHUB_OUTPUT
178-
echo "checks_skipped=3" >> $GITHUB_OUTPUT
179-
# Build JSON with proper escaping using jq
180-
RESULT=$(jq -n --arg reason "$REASON" '{
181-
checks: [
182-
{name: "Completeness", status: "skipped", result: $reason, reasoning: $reason, files: []},
183-
{name: "Scope", status: "skipped", result: $reason, reasoning: $reason, files: []},
184-
{name: "Traceability", status: "skipped", result: $reason, reasoning: $reason, files: []}
185-
],
186-
message: $reason
187-
}')
188-
echo "result=$(echo "$RESULT" | jq -c '.')" >> $GITHUB_OUTPUT
180+
echo "result=$RESULT" >> $GITHUB_OUTPUT
181+
echo "checks_passed=$CHECKS_PASSED" >> $GITHUB_OUTPUT
182+
echo "checks_failed=$CHECKS_FAILED" >> $GITHUB_OUTPUT
183+
echo "checks_skipped=$CHECKS_SKIPPED" >> $GITHUB_OUTPUT
189184
exit 0
190185
fi
191186
192-
# Initialize defaults
193-
CHECKS_PASSED=0
194-
CHECKS_FAILED=0
195-
CHECKS_SKIPPED=0
196-
PASSED="false"
197-
RESULT='{"checks":[],"message":"Review failed to complete"}'
198-
199-
EXEC="${{ steps.review.outputs.execution_file }}"
200-
if [ -f "$EXEC" ]; then
201-
# Debug: Show what we're working with
202-
echo "DEBUG: Checking execution file..."
203-
echo "DEBUG: File exists, size: $(wc -c < "$EXEC") bytes"
204-
echo "DEBUG: Message types in file:"
205-
jq -r '.[].type' "$EXEC" 2>/dev/null | sort | uniq -c || echo "DEBUG: jq failed"
206-
207-
# Get ALL text from assistant messages, joined together
208-
ALL_TEXT=$(jq -r '[.[] | select(.type=="assistant") | .message.content[]? | select(.type=="text") | .text] | join("\n")' "$EXEC" 2>/dev/null || echo "")
209-
echo "DEBUG: ALL_TEXT length: ${#ALL_TEXT}"
210-
echo "DEBUG: ALL_TEXT preview: ${ALL_TEXT:0:500}"
211-
# Strategy 1: Extract from ```json block
212-
JSON=$(echo "$ALL_TEXT" | sed -n '/```json/,/```/{/```/d;p;}' | head -100)
213-
214-
# Strategy 2: If no json block, try to find raw JSON object with "checks" key
215-
if ! echo "$JSON" | jq . >/dev/null 2>&1; then
216-
JSON=$(echo "$ALL_TEXT" | grep -oP '\{"checks":\s*\[.*\]\s*(,"message":[^}]*)?\}' | head -1)
217-
fi
218-
219-
# Strategy 3: Extract any JSON object starting with {"checks"
220-
if ! echo "$JSON" | jq . >/dev/null 2>&1; then
221-
JSON=$(echo "$ALL_TEXT" | perl -ne 'print if /\{"checks":\[/.../"message":/' | tr '\n' ' ')
222-
fi
223-
224-
# Strategy 4: Use Python for robust multiline JSON extraction
225-
if ! echo "$JSON" | jq . >/dev/null 2>&1; then
226-
JSON=$(echo "$ALL_TEXT" | python3 -c "import re,sys; t=sys.stdin.read(); m=re.search(r'\{[^{}]*\"checks\"[^{}]*\[.*?\][^{}]*\}',t,re.DOTALL); print(m.group() if m else '')" 2>/dev/null)
227-
fi
228-
229-
if echo "$JSON" | jq . >/dev/null 2>&1; then
230-
# Count checks by status
231-
CHECKS_PASSED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "passed")] | length')
232-
CHECKS_FAILED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "failed")] | length')
233-
CHECKS_SKIPPED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "skipped")] | length')
234-
235-
# Only pass if ALL checks passed (no failures)
236-
if [ "$CHECKS_FAILED" = "0" ] && [ "$CHECKS_PASSED" -gt "0" ]; then
237-
PASSED="true"
238-
elif [ "$CHECKS_FAILED" = "0" ] && [ "$CHECKS_PASSED" = "0" ] && [ "$CHECKS_SKIPPED" -gt "0" ]; then
239-
PASSED="true"
240-
fi
241-
242-
RESULT=$(echo "$JSON" | jq -c '.')
243-
fi
244-
fi
187+
extract_review_result "${{ steps.review.outputs.execution_file }}"
245188
246189
# Always output
247190
echo "passed=$PASSED" >> $GITHUB_OUTPUT

‎.github/actions/review-comment/action.yml‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -71,22 +71,16 @@ runs:
7171
STATUS_EMOJI="❌"
7272
else
7373
# All skipped
74-
STATUS_EMOJI="⚠️"
74+
STATUS_EMOJI="⏭️"
7575
fi
7676
77-
# Build summary line - split test: Requirements uses words only, others use emoji+number
78-
if [ "$REVIEW" = "Requirements" ]; then
79-
# Format A: Words only (no emoji)
80-
SUMMARY_PARTS="$CHECKS_PASSED passed • $CHECKS_FAILED failed • $CHECKS_SKIPPED skipped"
81-
else
82-
# Format B: Emoji + number only (compact)
83-
SUMMARY_PARTS="✅ $CHECKS_PASSED • ❌ $CHECKS_FAILED • ⚠️ $CHECKS_SKIPPED"
84-
fi
77+
# Build summary line with emojis
78+
SUMMARY_PARTS="✅ $CHECKS_PASSED • ❌ $CHECKS_FAILED • ⏭️ $CHECKS_SKIPPED"
8579
8680
# Build prompt link
8781
PROMPT_LINK=""
8882
if [ -n "$PROMPT_FILE" ]; then
89-
PROMPT_LINK="[View agent prompt](https://github.com/${{ github.repository }}/blob/${{ github.head_ref || github.ref_name }}/$PROMPT_FILE)"
83+
PROMPT_LINK="[View agent prompt](https://github.com/${{ github.repository }}/blob/${{ inputs.sha }}/$PROMPT_FILE)"
9084
fi
9185
9286
# Build checks table header
@@ -105,7 +99,7 @@ runs:
10599
case "$CHECK_STATUS" in
106100
"passed") CHECK_EMOJI="✅" ;;
107101
"failed") CHECK_EMOJI="❌" ;;
108-
*) CHECK_EMOJI="⚠️" ;;
102+
*) CHECK_EMOJI="⏭️" ;;
109103
esac
110104
111105
# Get files for this check
@@ -194,7 +188,7 @@ runs:
194188
# Create new comment
195189
{
196190
echo "$MARKER"
197-
echo "PR review updates from your Constellos agents. Based on linked issue(s), context files, and committed changes."
191+
echo "PR feedback updates from your reviewer agents. Based on linked issues, context files, and committed changes."
198192
echo ""
199193
echo "$AGENT_BLOCK"
200194
} > /tmp/comment.md

‎.github/actions/shared/extract-result.sh‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
#!/bin/bash
2+
# Shared logic for reviewer actions
3+
# Usage: source this file, then call the functions
4+
# Sets: CHECKS_PASSED, CHECKS_FAILED, CHECKS_SKIPPED, PASSED, RESULT
5+
6+
# Generate skipped result JSON
7+
# Usage: generate_skipped_result "reason" "Check1" "Check2" "Check3" ...
8+
# Sets: CHECKS_PASSED, CHECKS_FAILED, CHECKS_SKIPPED, PASSED, RESULT
9+
generate_skipped_result() {
10+
local REASON="$1"
11+
shift
12+
local CHECK_NAMES=("$@")
13+
14+
CHECKS_PASSED=0
15+
CHECKS_FAILED=0
16+
CHECKS_SKIPPED=${#CHECK_NAMES[@]}
17+
PASSED="true"
18+
19+
# Build checks array
20+
local CHECKS_JSON="["
21+
local FIRST=true
22+
for NAME in "${CHECK_NAMES[@]}"; do
23+
if [ "$FIRST" = "true" ]; then
24+
FIRST=false
25+
else
26+
CHECKS_JSON="$CHECKS_JSON,"
27+
fi
28+
CHECKS_JSON="$CHECKS_JSON$(jq -n --arg name "$NAME" --arg reason "$REASON" '{name: $name, status: "skipped", result: $reason, reasoning: $reason, files: []}')"
29+
done
30+
CHECKS_JSON="$CHECKS_JSON]"
31+
32+
RESULT=$(jq -n --argjson checks "$CHECKS_JSON" --arg reason "$REASON" '{checks: $checks, message: $reason}' | jq -c '.')
33+
}
34+
35+
# Extract review result from execution file
36+
# Usage: extract_review_result "$EXEC_FILE"
37+
# Sets: CHECKS_PASSED, CHECKS_FAILED, CHECKS_SKIPPED, PASSED, RESULT
38+
extract_review_result() {
39+
local EXEC="$1"
40+
41+
# Initialize defaults
42+
CHECKS_PASSED=0
43+
CHECKS_FAILED=0
44+
CHECKS_SKIPPED=0
45+
PASSED="false"
46+
RESULT='{"checks":[],"message":"Review failed to complete"}'
47+
48+
if [ -f "$EXEC" ]; then
49+
# Get ALL text from assistant messages, joined together
50+
ALL_TEXT=$(jq -r '[.[] | select(.type=="assistant") | .message.content[]? | select(.type=="text") | .text] | join("\n")' "$EXEC" 2>/dev/null || echo "")
51+
52+
# Strategy 1: Extract from ```json block
53+
JSON=$(echo "$ALL_TEXT" | sed -n '/```json/,/```/{/```/d;p;}' | head -100)
54+
55+
# Strategy 2: If no json block, try to find raw JSON object with "checks" key
56+
if ! echo "$JSON" | jq . >/dev/null 2>&1; then
57+
JSON=$(echo "$ALL_TEXT" | grep -oP '\{"checks":\s*\[.*\]\s*(,"message":[^}]*)?\}' | head -1)
58+
fi
59+
60+
# Strategy 3: Extract any JSON object starting with {"checks"
61+
if ! echo "$JSON" | jq . >/dev/null 2>&1; then
62+
JSON=$(echo "$ALL_TEXT" | perl -ne 'print if /\{"checks":\[/.../"message":/' | tr '\n' ' ')
63+
fi
64+
65+
# Strategy 4: Use Python for robust multiline JSON extraction
66+
if ! echo "$JSON" | jq . >/dev/null 2>&1; then
67+
JSON=$(echo "$ALL_TEXT" | python3 -c "import re,sys; t=sys.stdin.read(); m=re.search(r'\{[^{}]*\"checks\"[^{}]*\[.*?\][^{}]*\}',t,re.DOTALL); print(m.group() if m else '')" 2>/dev/null)
68+
fi
69+
70+
if echo "$JSON" | jq . >/dev/null 2>&1; then
71+
# Count checks by status
72+
CHECKS_PASSED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "passed")] | length')
73+
CHECKS_FAILED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "failed")] | length')
74+
CHECKS_SKIPPED=$(echo "$JSON" | jq '[.checks[]? | select(.status == "skipped")] | length')
75+
76+
# Only pass if ALL checks passed (no failures)
77+
if [ "$CHECKS_FAILED" = "0" ] && [ "$CHECKS_PASSED" -gt "0" ]; then
78+
PASSED="true"
79+
elif [ "$CHECKS_FAILED" = "0" ] && [ "$CHECKS_PASSED" = "0" ] && [ "$CHECKS_SKIPPED" -gt "0" ]; then
80+
# All skipped is considered a pass
81+
PASSED="true"
82+
fi
83+
84+
RESULT=$(echo "$JSON" | jq -c '.')
85+
fi
86+
fi
87+
}

0 commit comments

Comments
 (0)