chore(retro): reject "Evidence for" proposals in post-retro.sh#85
Conversation
PR Summary by QodoReject “Evidence for” retro proposals before filing GitHub issues
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
🤖 Finished Review · ✅ Success · Started 3:46 PM UTC · Completed 3:56 PM 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}
ReviewVerdict: approve — all prior review findings addressed; no medium+ issues remain. This is a re-review. The prior review (SHA
What changedThe PR adds a deterministic post-script gate in The regex patterns now correctly handle three cases as separate conditions:
The comment-assembly refactoring from a two-branch AnalysisCorrectness: Regex patterns are verified correct. The Security: The Test coverage: 14 new test cases cover: rejection, case insensitivity, "additional evidence" variant, false-positive guards (mid-title "evidence for", no issue ref), mixed proposals (both filtering and issue creation verified), content folding into summary, GHA command injection sanitization ( Architecture: Post-script filtering is the correct layer — the agent runs in a sandbox and cannot be prevented from producing evidence-for proposals, so the post-script gate provides defense-in-depth. This matches the pattern in Style: Follows established codebase conventions (variable naming, error handling, test structure, mock patterns, sanitization idioms). Documentation: Remaining low-severity notes
Previous runReviewVerdict: comment — one medium-severity finding worth noting; no blocking issues. The change adds a deterministic post-script gate to reject "Evidence for" proposals before they reach The comment-assembly refactoring from a two-branch Findings1. Regex colon alternative is unreachable (medium · correctness)File: The pattern Remediation: If if [[ "${TITLE_LOWER}" =~ ^evidence(:|[[:space:]]+(for|of)) ]] || \Alternatively, if the colon format does not occur in practice, remove it from the alternation to avoid dead code. 2. Mixed-proposals test does not verify issue creation (low · test-adequacy)File: The Remediation: Add a companion assertion (via 3. docs/retro.md does not mention evidence-for filtering (low · documentation-staleness)File: Line 14 states "Post-script creates GitHub issues from the agent's proposals" without mentioning that proposals matching evidence-for patterns are now filtered out and folded into the summary comment as evidence notes. Remediation: Update line 14 to note that evidence-for proposals are excluded from issue creation and included in the summary comment instead. 4. Raw title in EVIDENCE_NOTES markdown (low · markdown-injection)File: The |
![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.
fullsend-ai-review
Bot
added
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-squad pass (4 agents: Claude, Gemini, Codex). Most medium+ findings overlap with the existing automated review already on this PR (pipefail crash on jq | head -1, unreachable colon regex, weak SAFE_TITLE sanitization) — not re-posting those. One new finding below: the regex is also too broad in places, causing false-positive rejections of legitimate proposals, in addition to being too narrow for the "Evidence:" case already flagged.
rh-hemartin
force-pushed
the
fix/reject-evidence-for-proposals
branch
from
032b3f1 to
62e252e
Compare
The retro agent routinely ignores the prompt instruction not to file "Evidence for" issues (~335 filed in 4 days). Move enforcement from the LLM to a deterministic title-pattern gate in the post-script. Rejected proposals are logged as warnings and their content folded into the summary comment on the originating PR/issue. Closes fullsend-ai/fullsend#3881 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
rh-hemartin
force-pushed
the
fix/reject-evidence-for-proposals
branch
from
62e252e to
0478721
Compare
|
If I got that right, all feedback addressed. |
|
🤖 Finished Review · ✅ Success · Started 6:46 AM UTC · Completed 6:55 AM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Retro · ✅ Success · Started 3:02 PM UTC · Completed 3:09 PM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-squad pass (4 agents: Claude ×2, Gemini, Codex). 6 new MEDIUM findings posted inline — all verified against the head branch code. Existing review comments on sanitization weakness, pipefail/SIGPIPE, unreachable colon regex, and ^evidence broadness were already present and are not re-posted.
New comments posted: 6
Already posted (skipped): 5 (sanitization weakness, protected-path compliance, pipefail crash, echo option edge case, unbounded comment growth)
Updated in-place: 0
Assisted-by: Claude (review), Gemini (review), Codex (review)
| - **${TITLE}** (${TARGET_REPO}): $(jq -r ".proposals[$i].what_happened | split(\"\\n\")[0]" "${RESULT_FILE}")" | ||
| FILTERED_COUNT=$((FILTERED_COUNT + 1)) | ||
| continue | ||
| fi |
There was a problem hiding this comment.
[medium] correctness — unsanitized TITLE in EVIDENCE_NOTES
SAFE_TITLE is computed at lines 92-98 but the raw TITLE is used here in the evidence notes that get posted as a GitHub comment. If the title contains markdown metacharacters (**, [](url), <), they'll render in the comment body and could break formatting.
Suggestion: Use ${SAFE_TITLE} instead of ${TITLE}:
EVIDENCE_NOTES="${EVIDENCE_NOTES}
- **${SAFE_TITLE}** (${TARGET_REPO}): $(jq -r ...)"Flagged by 3/4 agents (Claude, Gemini, Codex)
| TITLE_LOWER=$(printf '%s' "${TITLE}" | tr '[:upper:]' '[:lower:]') | ||
| if [[ "${TITLE_LOWER}" =~ ^evidence[[:space:]]+(for|of)[[:space:]]+\# ]] || \ | ||
| [[ "${TITLE_LOWER}" =~ ^evidence: ]] || \ | ||
| [[ "${TITLE_LOWER}" =~ ^additional[[:space:]]+evidence ]]; then |
There was a problem hiding this comment.
[medium] correctness — ^additional[[:space:]]+evidence is broader than other patterns
The first pattern requires an issue reference (#) but this third pattern matches anything starting with "Additional evidence" — including legitimate titles like "Additional evidence-gathering capabilities for review agent". The asymmetry means this branch is more false-positive-prone than the others.
Suggestion: Either tighten to require a qualifier after "evidence" (e.g. ^additional[[:space:]]+evidence[[:space:]]+(for|of)) or add a code comment explaining why the broader match is intentional for this variant.
Flagged by 3/4 agents (Claude, Gemini, Codex)
| if [[ -n "${EVIDENCE_NOTES}" ]]; then | ||
| COMMENT=$(printf '%s\n\n### Evidence notes (not filed as issues)\n%s' "${COMMENT}" "${EVIDENCE_NOTES}") | ||
| fi | ||
|
|
There was a problem hiding this comment.
[medium] documentation — misleading comment about truncation strategy
Comment says "Truncate EVIDENCE_NOTES first if over" but the code truncates the entire COMMENT string positionally at 65000 chars. The comment describes intent the code doesn't implement.
Suggestion: Update to match actual behavior:
# GitHub comment limit is 65536 chars. Truncate from the end if over.
# Since evidence notes are appended last, they are trimmed first.Flagged by 2/4 agents (Claude coder, Claude researcher)
|
|
||
| # Fixture: a valid agent result with no proposals. | ||
| FIXTURE_NO_PROPOSALS='{ | ||
| "summary": "The retro analysis found no actionable improvements.", |
There was a problem hiding this comment.
[medium] test-coverage — missing tests for ^evidence: and evidence of regex branches
Two of the three regex branches have zero test coverage:
^evidence:— no fixture exercises a title like"Evidence: review coverage gap". If this branch were accidentally removed, no test would catch it.evidence of #N— the(for|of)alternation accepts "Evidence of #1234" but no fixture tests the "of" variant.
Suggestion: Add two fixtures:
FIXTURE_EVIDENCE_COLON='{ ... "title": "Evidence: review coverage gap" ... }'
FIXTURE_EVIDENCE_OF='{ ... "title": "Evidence of #1234: repeated pattern" ... }'with corresponding run_test_no_gh_call assertions.
Flagged by 3/4 agents (Claude coder, Claude researcher, Gemini)
|
|
||
| # --- Results --- | ||
|
|
||
| if [[ ${FAILURES} -gt 0 ]]; then |
There was a problem hiding this comment.
[medium] test-correctness — truncation length test passes vacuously if preceding test fails
If run_test_stdin "comment-truncated-at-limit" fails (script exits non-zero before posting), GH_STDIN_LOG is empty (0 bytes) and this length check passes vacuously — it never catches a real regression.
Suggestion: Guard with a non-empty check:
if [[ ! -s "${GH_STDIN_LOG}" ]]; then
echo "FAIL: comment-truncated-length — GH_STDIN_LOG is empty (precondition not met)"
FAILURES=$((FAILURES + 1))
elif [[ ${POSTED_LEN} -gt 65536 ]]; then
...Flagged by 1/4 agents (Claude researcher)
| SAFE_TITLE="${SAFE_TITLE//::/:}" | ||
| SAFE_TITLE="${SAFE_TITLE//%0A/}" | ||
| SAFE_TITLE="${SAFE_TITLE//%0a/}" | ||
| SAFE_TITLE="${SAFE_TITLE//%0D/}" | ||
| SAFE_TITLE="${SAFE_TITLE//%0d/}" | ||
| echo "::warning::proposal[$i] rejected — title matches evidence-for pattern: ${SAFE_TITLE}. Folding into summary." | ||
| EVIDENCE_NOTES="${EVIDENCE_NOTES} |
There was a problem hiding this comment.
[medium] maintainability — sanitization logic duplicated with inconsistent coverage
This block (lines 92-98) sanitizes newlines + :: → : + %0A/%0D. The issue-filing path (lines 131-135) sanitizes :: → empty + %0A/%0D but omits newlines and uses a different :: replacement (${TITLE//::/} vs ${TITLE//::/:}).
Both paths prevent GHA workflow command injection but diverge in behavior. A title with :: renders as foo:bar in the rejection warning but foobar in the filing output.
Suggestion: Extract into a shared block at the top of the loop, or at minimum align the two paths to use the same substitution rules.
Flagged by 3/4 agents (Claude coder, Gemini, Codex)
Retro: PR #85 — reject "Evidence for" proposals in post-retro.shPR #85 adds a deterministic post-script gate to filter out retro agent proposals whose titles match evidence-for patterns (e.g., "Evidence for #1234: ..."). The change is well-implemented with comprehensive test coverage (13+ test cases), proper GHA workflow command injection sanitization, and follows the established defense-in-depth pattern used by other agent post-scripts (review severity filtering, scribe content gates). The change addresses a real recurring problem: the retro-analysis skill explicitly instructs the agent not to file evidence-for proposals (SKILL.md line 145), but the agent ignores this frequently enough to require a post-script gate (referenced as fullsend-ai/fullsend#3881). Limitation: GitHub API access was unavailable during this retro, so I could not trace workflow runs, review comments, or search for existing open issues. Proposals below should be checked for duplicates before filing. Observations
Proposals filed
|
3 participants
Summary
post-retro.shthat rejects proposals matching "Evidence for" patterns before they reachgh issue create::warning::and their content folded into the summary comment posted on the originating PR/issueCloses fullsend-ai/fullsend#3881
What changed
scripts/post-retro.sh: Case-insensitive regex check in the filing loop rejects titles matching:^evidence (for|of|:)^additional evidenceevidence for #anywhereTitles like "Fix evidence gathering bug" pass through (the word "evidence" in non-matching context is not filtered).
scripts/post-retro-test.sh: 6 new test cases (rejection, case-insensitive, "Additional evidence" variant, false-positive guard, mixed proposals, content folding into summary). Mock updated to capturegh apistdin for body assertions.Test plan
bash scripts/post-retro-test.sh)make test)🤖 Generated with Claude Code