feat(agents): add action-hints footer to review, triage, and fix comments#53
feat(agents): add action-hints footer to review, triage, and fix comments#53rh-hemartin wants to merge 1 commit into
Conversation
|
🤖 Review · |
PR Summary by QodoAdd “Next steps” action-hints footers to review/triage specs and fix summaries
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
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}
| - **No general footer.** Do not repeat the outcome or include | ||
| boilerplate about pushes clearing the review. **Exception:** for | ||
| `request-changes` outcomes only, append an action-hints footer | ||
| after the findings: | ||
|
|
||
| ```markdown | ||
| --- | ||
| **Next steps:** | ||
| - `/fs-fix` — agent addresses review findings automatically | ||
| - `/fs-fix <your instruction>` — agent fixes with your specific guidance | ||
| - Push commits directly — review re-runs automatically on push | ||
| - `/fs-fix-stop` — disable automatic fix runs for this PR | ||
| ``` | ||
|
|
||
| Omit this footer for `approve`, `comment`, and `reject` outcomes. |
There was a problem hiding this comment.
3. Protected paths changed 📜 Skill insight § Compliance
This PR modifies files under protected governance/infrastructure paths (agents/, scripts/, skills/), which must not be auto-approved and require explicit human review. Even with an issue link, these paths require elevated scrutiny due to their ability to affect agent behavior and automation.
Agent Prompt
## Issue description
Protected governance/infrastructure paths were modified. These changes must be treated as requiring explicit human review (not auto-approval), and the PR should include clear justification/authorization.
## Issue Context
Protected-path changes include updates to agent/skill instructions and automation scripts.
## Fix Focus Areas
- agents/triage.md[318-325]
- scripts/process-fix-result.py[1-198]
- skills/pr-review/SKILL.md[845-859]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
rh-hemartin
force-pushed
the
fix/1789-review-action-hints
branch
from
cf90488 to
a2c10f3
Compare
rh-hemartin
changed the title
|
🤖 Finished Review · ✅ Success · Started 6:47 AM UTC · Completed 7:02 AM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: All modified files are in protected paths requiring manual review. The PR adds new agent functionality. |
![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
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review — #53
Agents: 5 (Claude ×3, Gemini, Codex) | Findings: 14 total, 5 unique MEDIUM+ after dedup and verification against the PR branch
Context: extraction gap
The existing jsonschema dependency finding (already posted by qodo) is not new to this PR — it's a consequence of post-fix.sh being extracted from fullsend with the process-fix-result.py call baked in, while the script itself wasn't extracted. This PR fills that gap. The fix should still land here since this is the PR introducing the script, but it's not a regression.
High (1) — not inline
Tests not wired into CI
scripts/process-fix-result-test.py adds 27 tests but is not listed in the Makefile script-test target. The PR branch Makefile is identical to main. CI runs make script-test, which only runs shell-based test files — these Python tests will never execute in CI.
Suggested fix: Add to the Makefile script-test target:
$(call run-timed,python3 scripts/process-fix-result-test.py)The CI workflow already installs jsonschema==4.23.0 via uv pip install --system, so the dependency is available in the test path.
Medium (2) — posted inline
- Truncation silently destroys the footer (
process-fix-result.py:91) — 2/5 agents consensus - stdout/stderr capture lacks try/finally (
process-fix-result-test.py:170) — 3/5 agents consensus
Already posted (skipped)
- jsonschema dependency (qodo, line 22) — already covered
- GHA workflow command injection ×2 (lines 108, 147) — already covered as low
Assisted-by: Claude (review), Gemini (review), Codex (review)
rh-hemartin
force-pushed
the
fix/1789-review-action-hints
branch
2 times, most recently
from
4c077f0 to
461a512
Compare
|
🤖 Review · |
…ents Add "Next steps" footers showing available slash commands after review, triage, and fix agent comments so users know what actions are available. Review footer (request-changes only): /fs-fix, /fs-fix-stop, push. Triage footer (sufficient only): /fs-code, appended by post-triage.sh. Fix footer: /fs-review, /fs-fix, push — survives body truncation by reserving space for the suffix before cutting the main body. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
rh-hemartin
force-pushed
the
fix/1789-review-action-hints
branch
from
461a512 to
a57e3b5
Compare
|
🤖 Finished Review · ✅ Success · Started 8:04 AM UTC · Completed 8:21 AM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad pass (3 agents: Claude, Claude, Gemini) on the rebased state. My two prior MEDIUM findings (truncation destroying the footer, missing try/finally guards) are confirmed fixed. Posting the 4 new unique MEDIUM findings inline — all relate to the same theme: the footer feature was hardened for triage/fix but the review-agent path and the "don't suggest commands for uninstalled agents" request were left unaddressed, plus one unrelated finding on the comment-length constant.
| # --- Append action-hints footer (sufficient only) --- | ||
|
|
||
| if [[ "${ACTION}" == "sufficient" ]]; then |
There was a problem hiding this comment.
[MEDIUM] Footer suggests /fs-code without checking a code agent is actually installed
ralphbean's review comment on the original agents/triage.md thread asked for this footer to be added "conditionally only when the code agent is installed... check that in the post script, so we don't suggest commands for an agent that isn't in place." The rebase moved the footer logic here into deterministic bash as requested (good), but it's still unconditional on ACTION == "sufficient" — there's no check against the target repo's configured harness sources (e.g. whether harness/code.yaml/harness/fix.yaml is actually included) before appending the hint. No reply or follow-up commit addresses this half of the request; it reads as silently dropped rather than consciously deferred.
Suggestion: Add a check in this script (e.g. reading the resolved repo config) to confirm a code-agent harness is present before appending the footer, or get an explicit acknowledgment that this was intentionally deferred so it isn't lost.
| - **No general footer.** Do not repeat the outcome or include | ||
| boilerplate about pushes clearing the review. **Exception:** for | ||
| `request-changes` outcomes only, append an action-hints footer | ||
| after the findings: |
There was a problem hiding this comment.
[MEDIUM] Review-agent footer has no deterministic enforcement, unlike the now-hardened triage/fix paths
This footer is specified only as a prose instruction here, with zero runtime enforcement in scripts/post-review.sh — no logic there appends, verifies, or falls back on it. This is inconsistent with the triage case (scripts/post-triage.sh, deterministically appended in bash) and the fix case (scripts/process-fix-result.py's FOOTER constant, deterministically appended in Python), both hardened specifically in response to review feedback on this same PR. A model that omits or misplaces the footer on a request-changes outcome ships silently, with no test able to catch it — undermining the whole point of the feature (reliable command discoverability).
Suggestion: Move the footer append into scripts/post-review.sh, mirroring post-triage.sh: deterministically append the footer when outcome == "request-changes" after the agent returns its verdict, rather than relying on the model to reproduce the markdown block verbatim.
| - `/fs-fix-stop` — disable automatic fix runs for this PR | ||
| ``` | ||
|
|
||
| Omit this footer for `approve`, `comment`, and `reject` outcomes. |
There was a problem hiding this comment.
[MEDIUM] Footer-suppression rule doesn't address the pre-existing failure outcome
"Omit this footer for approve, comment, and reject outcomes" doesn't mention failure (the review agent's own error state, action table below at line ~877), leaving the model to guess whether the footer applies there. Since failure represents the agent's own error rather than a real code verdict, the footer should almost certainly never apply — but the instruction doesn't say so.
Suggestion: Extend the omit-list to explicitly include failure.
| return "\n".join(sections) | ||
|
|
||
|
|
||
| MAX_COMMENT_LENGTH = 32768 |
There was a problem hiding this comment.
[MEDIUM] premature-decision — MAX_COMMENT_LENGTH = 32768 is exactly half of GitHub's real comment limit, with no stated rationale
GitHub's documented max PR/issue comment body length is 65536 characters; this hardcodes half that, with no comment explaining the choice, and no repo-wide convention it was copied from (sibling scripts use unrelated constants for different purposes, e.g. post-scribe.sh's MAX_COMMENT_LEN=2000 bounds a per-topic field, not a whole comment body).
I checked whether this might intentionally reserve headroom for content appended after posting (e.g. a later edit/PATCH to this same comment) — it doesn't apply here: post_summary() posts via a plain gh pr comment ... --body-file - call with no marker and no --edit-last, and nothing else in this repo (post-fix.sh or any workflow) ever edits or appends to this comment afterward. Each fix iteration posts an entirely new comment. So there's no code-backed reason found for reserving half the limit — it silently discards roughly half of the actual usable space.
Suggestion: Size this close to GitHub's real 65536-char limit (the existing budget = MAX_COMMENT_LENGTH - len(suffix) - len(truncation_notice) logic already accounts for the footer/attribution suffix), e.g. MAX_COMMENT_LENGTH = 65536 with a comment citing GitHub's documented limit. If a safety margin is wanted for encoding/byte-vs-character discrepancies, use a small fixed buffer with a comment explaining it, not an unstated 50% cut.
3 participants
Summary
request-changesoutcomes, showing/fs-fix,/fs-fix <instruction>, push, and/fs-fix-stopsufficientaction, showing/fs-codeand/fs-code <instruction>/fs-review,/fs-fix <instruction>, and pushCloses fullsend-ai/fullsend#1789
Test plan
process-fix-result-test.pypasses (27 tests)request-changesoutcome — verify footer appearssufficientaction — verify footer appearsapprove,comment,reject,insufficient,duplicate,prerequisites,questionoutcomes🤖 Generated with Claude Code