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. Missing jsonschema dependency
|
![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
| import json | ||
| import os | ||
| import subprocess | ||
| import sys | ||
|
|
||
| import jsonschema | ||
|
|
There was a problem hiding this comment.
4. Missing jsonschema dependency 🐞 Bug ☼ Reliability
scripts/process-fix-result.py unconditionally imports and uses jsonschema, but the fix post-script invokes it on the GitHub Actions runner without ensuring jsonschema is installed. This can crash the post-fix step (after a successful push), preventing the fix summary comment from being posted and potentially failing the workflow.
Agent Prompt
## Issue description
`scripts/process-fix-result.py` imports `jsonschema` unconditionally and calls `jsonschema.validate(...)`. The fix pipeline’s post-script (`scripts/post-fix.sh`) runs this script on the Actions runner, but does not install or verify the presence of the `jsonschema` package. If the runner environment doesn’t already have `jsonschema`, the post-fix step will fail with `ModuleNotFoundError` after pushing commits, and the summary comment won’t be posted.
## Issue Context
- The harness already validates agent output against a schema during the validation loop, so the extra validation in `process-fix-result.py` is defense-in-depth but introduces a runtime dependency.
- The repo’s own schema-validation helper explicitly checks for `jsonschema` and fails if it’s missing, implying it is not guaranteed to be present by default.
## Fix Focus Areas
Choose one (or combine):
1) **Remove the runtime dependency**: drop JSON Schema validation in `process-fix-result.py` (or replace with minimal manual validation of required fields/types using stdlib only), since the harness validation loop already enforces the schema.
2) **Make validation best-effort**: wrap the `jsonschema` import in `try/except ImportError`; if missing, log a warning and continue posting the summary (or exit with code `2` so `post-fix.sh` treats it as non-fatal).
3) **Install dependency in runner**: update `scripts/post-fix.sh` to install `jsonschema` if missing (similar to how it installs `pre-commit`).
- scripts/process-fix-result.py[16-22]
- scripts/process-fix-result.py[162-179]
- scripts/post-fix.sh[317-362]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…ents Users receiving agent comments don't know what commands are available. Add contextual "Next steps" footers so available actions are discoverable at the decision point — review (request-changes only), triage (sufficient only), and fix (always). Closes #1789, closes #2493, closes #2494 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
cf90488 to
a2c10f3
Compare
rh-hemartin
changed the title
|
🤖 Finished Review · ✅ Success · Started 6:47 AM UTC · Completed 7:02 AM UTC |
ReviewFindingsMedium
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}
| - Do not present unverified assumptions with certainty. Convey uncertainty when appropriate. | ||
| - Write in second person ("you") addressing the reporter. Do not use first person ("I") — the comment is from the triage system, not an individual. | ||
| - If you include `label_actions`, the pipeline appends your label reason to the comment automatically — do not include label justifications in the `comment` field yourself. | ||
| - **Action hints footer (sufficient action only):** When `action` is `sufficient`, append the following footer to the end of the `comment` field. Omit it for all other actions (`insufficient`, `duplicate`, `prerequisites`, `question`). |
There was a problem hiding this comment.
[low] logic-error
Action-hints footer instruction placed under general 'Comment content rules' section rather than within the 'sufficient' action definition. The agent will still process it, but locality would improve discoverability.
| data = { | ||
| "summary": "Fixed.", | ||
| "tests_passed": True, | ||
| "actions": [ |
There was a problem hiding this comment.
[low] test-inadequate
test_action_hints_footer_present only tests with fix actions. Missing: disagree-only case, full footer item verification (no assertion for 'Push commits directly' line).
| "actions": [ | ||
| {"type": "fix", "finding": "bug", "description": "Fixed"}, | ||
| ], | ||
| } |
There was a problem hiding this comment.
[low] edge-case
test_long_body_truncated verifies dry-run path but does not assert truncation to MAX_COMMENT_LENGTH.
| "actions": [ | ||
| {"type": "fix", "finding": "bug", "description": "Fixed"}, | ||
| ], | ||
| } |
There was a problem hiding this comment.
[low] edge-case
stdout/stderr capture uses direct assignment without try/finally. If post_summary raises, sys.stdout stays redirected for remaining tests.
| print( | ||
| "Usage: process-fix-result.py <fix-result.json> <owner/repo> <pr-number> [--dry-run]", | ||
| file=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
[low] GHA workflow command injection
Schema validation error e.message interpolated into ::error:: without sanitizing newlines or :: sequences. Practical impact limited to workflow log annotation.
Suggested fix: Sanitize: e.message.replace('\r','').replace('\n',' ').replace('::',': :')
|
|
||
|
|
||
| MAX_COMMENT_LENGTH = 32768 | ||
|
|
There was a problem hiding this comment.
[low] GHA workflow command injection
e.stderr from gh CLI interpolated into ::warning:: without sanitization. GitHub API error responses could reflect user-controlled content.
Suggested fix: Same sanitization approach as the e.message finding.
fullsend-ai-review
Bot
added
requires-manual-review
1 participant
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 #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