feat(retro): post evidence comments on related issues#82
Conversation
PR Summary by QodoRetro: post corroborating evidence as comments on existing issues
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
🤖 Review · |
rh-hemartin
force-pushed
the
feat/60-evidence-comments
branch
from
431a8a4 to
7ef19ed
Compare
|
🤖 Finished Review · ✅ Success · Started 8:47 AM UTC · Completed 8:56 AM UTC |
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # Post evidence comments on referenced issues. | ||
| EVIDENCE_COUNT=$(jq '.evidence_comments // [] | length' "${RESULT_FILE}") | ||
| if [[ "${EVIDENCE_COUNT}" -gt 0 ]]; then | ||
| echo "Posting ${EVIDENCE_COUNT} evidence comment(s)" | ||
| for i in $(seq 0 $((EVIDENCE_COUNT - 1))); do | ||
| ISSUE_URL=$(jq -r ".evidence_comments[$i].issue_url" "${RESULT_FILE}") | ||
| EVIDENCE_BODY=$(jq -r ".evidence_comments[$i].body" "${RESULT_FILE}") | ||
|
|
||
| # Parse repo and number from issue URL. | ||
| EVIDENCE_REPO=$(echo "${ISSUE_URL}" | sed -E 's#https://github.com/##; s#/issues/.*##') | ||
| EVIDENCE_NUMBER=$(basename "${ISSUE_URL}") | ||
|
|
||
| SAFE_URL="${ISSUE_URL//::/}" | ||
| echo " Commenting on ${EVIDENCE_REPO}#${EVIDENCE_NUMBER}" | ||
| EVIDENCE_OUTPUT="" | ||
| EVIDENCE_EXIT=0 | ||
| EVIDENCE_OUTPUT=$(jq -nc --arg body "${EVIDENCE_BODY}" '{body: $body}' | gh api \ | ||
| "repos/${EVIDENCE_REPO}/issues/${EVIDENCE_NUMBER}/comments" \ | ||
| --input - 2>&1) || EVIDENCE_EXIT=$? | ||
|
|
||
| if [[ ${EVIDENCE_EXIT} -ne 0 ]]; then | ||
| if echo "${EVIDENCE_OUTPUT}" | grep -qE "HTTP (401|403)"; then | ||
| SAFE_OUTPUT="${EVIDENCE_OUTPUT//::/}" | ||
| SAFE_OUTPUT="${SAFE_OUTPUT//%0A/}" | ||
| SAFE_OUTPUT="${SAFE_OUTPUT//%0a/}" | ||
| SAFE_OUTPUT="${SAFE_OUTPUT//%0D/}" | ||
| SAFE_OUTPUT="${SAFE_OUTPUT//%0d/}" | ||
| echo "::warning::Could not post evidence comment on ${EVIDENCE_REPO}#${EVIDENCE_NUMBER}: insufficient permissions (${SAFE_OUTPUT}). Skipping." | ||
| else | ||
| SAFE_OUTPUT="${EVIDENCE_OUTPUT//::/}" | ||
| SAFE_OUTPUT="${SAFE_OUTPUT//%0A/}" | ||
| SAFE_OUTPUT="${SAFE_OUTPUT//%0a/}" | ||
| SAFE_OUTPUT="${SAFE_OUTPUT//%0D/}" | ||
| SAFE_OUTPUT="${SAFE_OUTPUT//%0d/}" | ||
| echo "ERROR: failed to post evidence comment on ${EVIDENCE_REPO}#${EVIDENCE_NUMBER}: ${SAFE_OUTPUT}" | ||
| exit 1 | ||
| fi | ||
| fi | ||
| done | ||
| fi | ||
|
|
There was a problem hiding this comment.
3. Protected paths modified 📜 Skill insight § Compliance
This PR modifies protected governance/infrastructure paths (scripts/ and skills/), which require explicit human review and must not be auto-approved. Even with an issue link, this should be surfaced as a protected-path change needing manual approval.
Agent Prompt
## Issue description
Protected governance/infrastructure paths were modified and must be explicitly flagged for human review.
## Issue Context
This PR changes automation scripts and agent skill instructions.
## Fix Focus Areas
- scripts/post-retro.sh[125-165]
- skills/retro-analysis/SKILL.md[121-183]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Evidence comments: no evidence_comments field — no extra API calls. | ||
| run_test_stdout "no-evidence-field" \ | ||
| "${FIXTURE_NO_PROPOSALS}" \ | ||
| "Post-retro complete." |
There was a problem hiding this comment.
5. No-evidence test doesn't assert 🐞 Bug ⚙ Maintainability
The new "no-evidence-field" test claims to ensure no extra API calls occur when evidence_comments is absent, but it only checks stdout for "Post-retro complete." and never inspects the gh call log. This test can pass even if evidence-comment gh api calls are accidentally made.
Agent Prompt
## Issue description
The `no-evidence-field` test is intended to validate that no evidence-comment API calls are made, but it uses `run_test_stdout`, which does not check `${GH_LOG}` at all.
## Issue Context
- `run_test()` verifies `${GH_LOG}` patterns.
- `run_test_stdout()` only greps stdout, so it cannot validate side effects like extra `gh api .../comments` calls.
## Fix Focus Areas
- scripts/post-retro-test.sh[200-251]
- scripts/post-retro-test.sh[325-329]
### Suggested implementation direction
- Either:
- Add a new helper that asserts `${GH_LOG}` does *not* contain a substring (e.g., `target-repo/issues/42/comments`), or
- Extend `run_test_stdout` to accept an optional `forbidden_pattern` (or expected call count) and assert against `${GH_LOG}`.
- Update `no-evidence-field` to assert the gh log contains only the originating summary comment call and no other `/comments` endpoints.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewVerdict: Approve All five findings from the prior review have been addressed:
What looks good
Minor observations1. Evidence ordering relative to summary comment (low)Evidence comments are posted before the summary comment (between proposal filing and summary posting). A non-permission fatal error (e.g., HTTP 404 if a referenced issue was deleted between analysis and posting) would prevent the summary from being posted. This follows the same pattern as proposal filing (which also can block the summary), but evidence targets external repos the script doesn't own, making unexpected responses slightly more likely. Consider moving evidence posting after the summary comment, or making all evidence errors non-fatal since evidence comments are supplementary data points. 2. No test for pre-validation rejection (low)The pre-validation loop correctly rejects malformed
Previous runReviewVerdict: Request changes Clean implementation that follows established patterns well — the error handling, test structure, and schema design are all consistent with the existing codebase. Two issues need attention before merge. Findings1.
|
![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.
When the retro agent finds evidence corroborating an existing open issue, the post-script now posts that evidence as a comment directly on the referenced issue instead of burying it in the originating PR's summary. Closes #60 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
rh-hemartin
force-pushed
the
feat/60-evidence-comments
branch
from
7ef19ed to
7a03769
Compare
|
🤖 Finished Review · ✅ Success · Started 11:57 AM UTC · Completed 12:28 PM UTC |
fullsend-ai-review
Bot
added
the
requires-manual-review
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
LGTM. Previous review findings all addressed in the latest push.
|
Closing as discussed in our meeting. We will prevent retro from filing evidence directly in the post-retro.sh code |
2 participants
Summary
evidence_commentsarray to the retro result schema (issue_url+body)evidence_commentsinstead of burying evidence in the summary fieldCloses #60
Test plan
make testsuite passes (all repo script tests)🤖 Generated with Claude Code