feat: add generic critique agent#87
Conversation
Add a platform-agnostic adversarial reviewer agent that validates refinement plans against quality dimensions. Includes harness config, sandbox policy (read-only GitHub API), pre/post scripts, result schema with description_clarity and project_routing dimensions, create-children.sh for child issue creation (GitHub sub-issues + Jira hierarchy), and test suite (35 tests). Depends on shared scripts from PR #11 (explore agent) — comment-helpers.sh, pre-explore.sh, and markdown-to-adf.py. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
PR Summary by QodoAdd generic critique agent with verdict routing and child-issue creation
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
|
🤖 Finished Review · ✅ Success · Started 5:47 PM UTC · Completed 6:03 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}
Review — #87
SummaryThe latest commit ( The three medium-severity findings from the prior review remain unresolved and are carried forward unchanged. No new issues were introduced by the latest commit. The PR is functional for the primary GitHub flow; the remaining findings affect Jira integration completeness, display accuracy for edge-case verdicts, and defense-in-depth security hardening. Medium1. GHA workflow command injection via unsanitized interpolated values (unresolved from prior review)
An attacker controlling the refine agent's output JSON (e.g., via prompt injection through an issue description) can craft child titles containing Remediation: Import or duplicate 2. Schema does not require
|
| Verdict | Request Changes |
| Reviewed | 6d0c112 |
| Findings | 1 high, 3 medium |
| Re-review | 5 of 9 prior findings addressed |
Summary
The fix commit addresses the most critical findings from the prior review: the cross-platform parent key mismatch (via resolve_github_parent_number()), the unguarded source, the disallowedTools bypass, the FULLSEND_OUTPUT_FILE mismatch, sandbox GH_TOKEN leakage, and the refine-approved/refine-escalated label disambiguation. The escalation history mutation is also corrected — verdict is now preserved as revise with an escalated flag rather than being rewritten to approved.
However, three prior findings remain unresolved (Makefile test registration, GHA workflow command injection, schema/post-script assessment contract mismatch), and one new logic gap was found (Jira label handling is only implemented for one of four verdict paths).
High
1. Test suite not registered in Makefile (unresolved from prior review)
scripts/post-critique-test.sh (35 tests) is not added to the script-test target in the Makefile. CI runs make test via the script-test target. Every other agent's test suite is registered (triage, prioritize, code, review, fix, retro, scribe, validate-output-schema). Critique tests are never executed in CI.
Remediation: Add $(call run-timed,bash scripts/post-critique-test.sh) to the script-test target in the Makefile.
Medium
2. GHA workflow command injection via unsanitized interpolated values (unresolved from prior review)
create-children.sh, post-critique.sh, and pre-critique.sh interpolate unsanitized values into ::notice:: and ::warning:: GHA workflow commands. The repo has an established sanitize_gha() pattern in post-scribe.sh that strips ::, %0A, %0D, and newline characters.
3. Schema does not require assessment for non-approved verdicts (unresolved from prior review)
The JSON schema's allOf conditional validation only requires assessment when verdict is approved.
4. Jira label handling missing for 3 of 4 verdict paths
The revise under limit verdict path adds labels to Jira issues. The other three verdict paths only add labels on GitHub.
Low
- Tests re-implement logic
- Missing token masking
- Implicit contract in
create-children.sh
Previous run
Review — #87
| Verdict | Request Changes |
| Reviewed | 23d7556 |
| Findings | 3 high, 6 medium |
Summary
This PR adds a well-structured critique agent following the repo's established pre-script → sandbox → post-script pipeline. The agent prompt is thorough, the schema uses conditional validation correctly, and the test suite covers core verdict routing. However, there are several code-level issues.
Previous run
Review — #87
| Verdict | Request Changes |
| Reviewed | 6d0c112 |
| Findings | 1 high, 3 medium |
| Re-review | 5 of 9 prior findings addressed |
Summary
The fix commit addresses the most critical findings from the prior review: the cross-platform parent key mismatch (via resolve_github_parent_number()), the unguarded source, the disallowedTools bypass, the FULLSEND_OUTPUT_FILE mismatch, sandbox GH_TOKEN leakage, and the refine-approved/refine-escalated label disambiguation. The escalation history mutation is also corrected — verdict is now preserved as revise with an escalated flag rather than being rewritten to approved.
However, three prior findings remain unresolved (Makefile test registration, GHA workflow command injection, schema/post-script assessment contract mismatch), and one new logic gap was found (Jira label handling is only implemented for one of four verdict paths).
High
1. Test suite not registered in Makefile (unresolved from prior review)
scripts/post-critique-test.sh (35 tests) is not added to the script-test target in the Makefile. CI runs make test via the script-test target. Every other agent's test suite is registered (triage, prioritize, code, review, fix, retro, scribe, validate-output-schema). Critique tests are never executed in CI.
Remediation: Add $(call run-timed,bash scripts/post-critique-test.sh) to the script-test target in the Makefile.
Medium
2. GHA workflow command injection via unsanitized interpolated values (unresolved from prior review)
create-children.sh, post-critique.sh, and pre-critique.sh interpolate unsanitized values into ::notice:: and ::warning:: GHA workflow commands. The repo has an established sanitize_gha() pattern in post-scribe.sh that strips ::, %0A, %0D, and newline characters. The highest-risk emissions are:
create-children.sh:$titleand$summary(from agent JSON.children[].title),$body(raw Jira API error response — can contain arbitrary content),$CHILD_TITLE,$CREATED_KEYSpost-critique.sh:$CHILD_SUMMARY(contains API-returned issue keys),$ISSUE_KEYpre-critique.sh:$ISSUE_KEY,$ISSUE_SOURCE
An attacker controlling the refine agent's output JSON (e.g., via prompt injection through an issue description) can craft child titles containing %0A::set-env name=... sequences to inject additional workflow commands.
Remediation: Import or duplicate sanitize_gha() from post-scribe.sh and apply to all interpolated variables in workflow commands.
3. Schema does not require assessment for non-approved verdicts (unresolved from prior review)
The JSON schema's allOf conditional validation only requires assessment when verdict is approved. The revise and needs_input conditional blocks do not include assessment in their required arrays. However, post-critique.sh unconditionally reads .assessment.overall // 0 and displays it in the comment table for all verdict paths. When the agent validly omits assessment for a revise or needs_input result (which the schema allows), the posted comment shows "Score: 0/100", which is misleading — it implies the plan scored zero rather than that the score was not computed.
Remediation: Either (a) add assessment to the required arrays for revise and needs_input in the schema, or (b) conditionally omit the Score row from the comment table when .assessment is null/absent.
4. Jira label handling missing for 3 of 4 verdict paths
The revise under limit verdict path adds labels to Jira issues via a curl PUT to /rest/api/3/issue/${ISSUE_KEY} with label update payload. The other three verdict paths — approved, revise at limit (escalation), and needs_input — only add labels on GitHub via add_label. Jira issues will not receive refine-approved, refine-needs-human, refine-escalated, or refine-needs-input labels, breaking label-based pipeline signaling for Jira-sourced work items.
The revise under limit path proves the Jira label pattern works, so the omission in the other paths appears to be an oversight.
Remediation: Add Jira label handling (matching the existing pattern in the revise under limit path) to the approved, escalation, and needs_input verdict paths.
Low
- Tests re-implement logic —
post-critique-test.shdefines its own copies ofdetermine_reply_target,determine_verdict_action, anddetermine_labelsrather than testing the actualpost-critique.shcode paths. If the script's inline logic diverges from the test's copy, tests will pass while the real script has bugs. Partially justified by thecomment-helpers.shdependency. - Missing token masking —
post-critique.shdoes not call::add-mask::forGH_TOKEN,PUSH_TOKEN, orJIRA_API_TOKEN. Every other post-script in the repo masks its tokens at the top of the file (post-review.sh,post-fix.sh,post-scribe.sh,post-retro.sh,post-code.sh). - Implicit contract in
create-children.sh— The script assumesRESULT_FILEcontains a.childrenarray (from the refine result) but has no validation beyond JSON parsing.post-critique.shreassignsRESULT_FILEto the refine result path before sourcing, making the contract fragile — passing the wrong file would result in silent zero-child creation.
Previous run (2)
Review — #87
| Verdict | Request Changes |
| Reviewed | 23d7556 |
| Findings | 3 high, 6 medium |
Summary
This PR adds a well-structured critique agent following the repo's established pre-script → sandbox → post-script pipeline. The agent prompt is thorough, the schema uses conditional validation correctly, and the test suite covers core verdict routing. However, there are several code-level issues — a confirmed logic bug in cross-platform issue creation, missing CI registration for the test suite, GHA workflow command injection gaps, and defense-in-depth issues in the sandbox configuration — that require resolution.
High
1. Cross-platform parent key mismatch — create-children.sh
When ISSUE_SOURCE=jira (parent key is e.g. PROJ-123) and a child has target_platform=github, the code passes the Jira key as a GitHub issue number to the sub-issues API: repos/{repo}/issues/PROJ-123/sub_issues. This call returns 404/422 and is silently suppressed by 2>/dev/null || warning. The child issue is created but never linked to its parent.
Remediation: When the parent is on a different platform than the child, skip sub-issue linking or require an explicit GITHUB_ISSUE_NUMBER for cross-platform scenarios.
2. Test suite not registered in Makefile
scripts/post-critique-test.sh (35 tests) is not added to the Makefile script-test target. CI runs make test, so critique agent tests will not execute. Every other agent's test suite is registered.
Remediation: Add $(call run-timed,bash scripts/post-critique-test.sh) to the script-test target in the Makefile.
3. Unguarded source of missing dependency
post-critique.sh line 36 runs source "${SCRIPT_DIR}/comment-helpers.sh" with set -euo pipefail active. This file is provided by PR #11 (acknowledged in the PR body) but does not exist on main. If this PR merges first, post-critique.sh crashes on every invocation with no diagnostic message.
Remediation: Add a guard: if [[ ! -f "${SCRIPT_DIR}/comment-helpers.sh" ]]; then echo "ERROR: comment-helpers.sh not found (requires PR #11)" >&2; exit 1; fi
Medium
4. GHA workflow command injection via unsanitized interpolated values
create-children.sh and post-critique.sh interpolate agent-derived and API-response values (issue titles, verdict strings, scores) into ::notice:: and ::warning:: GHA workflow commands without sanitizing for :: sequences, %0A/%0D encoded newlines, or raw newlines. The repo already has a sanitization pattern in post-scribe.sh (sanitize_gha()).
Remediation: Apply the same sanitization pattern — strip ::, %0A, %0D, and newline characters from all interpolated variables before they appear in workflow commands.
5. disallowedTools pattern bypass for implicit gh api POST
The agent blocks Bash(gh api *POST*) but gh api endpoint -F field=value performs POST implicitly without containing the literal string "POST". The network policy (access: read-only) provides a backstop, but the application-level control has a gap.
Remediation: Add patterns for -F, -f, --input flags, or use a blanket Bash(gh api *) block as the fix agent does.
6. FULLSEND_OUTPUT_FILE mismatch in harness config
harness/critique.yaml sets FULLSEND_OUTPUT_FILE: critique-result.json in env.runner, but the agent prompt writes agent-result.json and the post-script searches for agent-result.json. Currently benign (the validation script defaults to agent-result.json in the sandbox), but creates a latent inconsistency. No other agent harness sets this variable to a non-default value while the agent writes agent-result.json.
Remediation: Remove the FULLSEND_OUTPUT_FILE override from the harness, or align the agent prompt to write critique-result.json.
7. GH_TOKEN passed to sandbox without clear need
The sandbox receives GH_TOKEN via env.sandbox, but the agent reads pre-staged context files (issue-context.json, refine-result.json, etc.) and the prompt does not instruct gh api calls for data retrieval. Passing secrets to a sandbox when they are not needed violates least privilege.
Remediation: Remove GH_TOKEN from env.sandbox. If the agent needs read-only GitHub API access, consider minting a read-only scoped token.
8. refine-approved label has dual meaning
The label is applied both for genuine approvals (line 162) and for max-rounds escalation (line 192), where the critique agent still has unresolved concerns. Downstream automation triggering on refine-approved cannot distinguish between the two cases.
Remediation: Use a distinct label for escalation (e.g., refine-escalated) or ensure AUTO_CREATE is not acted upon in the escalation path.
9. Schema does not require assessment for revise/needs_input but post-script reads it unconditionally
The JSON schema only requires assessment for the approved verdict. But post-critique.sh unconditionally reads .assessment.overall // 0 for all verdicts, and the history tracking records overall_score for every round. If the agent omits assessment for a revise result (which the schema allows), the comment displays "Score: 0/100".
Remediation: Either add assessment to the required arrays for all verdict types in the schema, or handle missing assessment gracefully in the post-script.
Low
- Missing
icons/critique.png—docs/critique.mdreferencesicons/critique.pngbut the file is not in the PR or on main. - Escalation path rewrites history — When max rounds reached, critique history changes the last round's verdict from
revisetoapprovedwith anescalated: trueflag, which could mislead downstream consumers checking.verdict == "approved". - Missing token masking — Other post-scripts use
::add-mask::for write tokens;post-critique.shdoes not maskGH_TOKEN/PUSH_TOKEN/JIRA_API_TOKEN. Baseline inconsistency (post-triage.shalso lacks masking).
![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.
- Block all gh api calls in sandbox (match fix agent pattern) - Remove FULLSEND_OUTPUT_FILE override and GH_TOKEN from sandbox - Guard shared script dependencies with clear PR #11 messages - Fix cross-platform GitHub parent linking via resolve_github_parent_number - Default null acceptance_criteria to empty array in create-children.sh - Add GitHub sub-issue deduplication alongside existing Jira dedup - Use refine-escalated label instead of refine-approved on max rounds - Preserve original verdict in critique history on escalation - Clarify sandbox network capabilities in user docs Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 6:12 PM UTC · Completed 6:42 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Fix SC2034/SC2015/SC2129 shellcheck failures - Upgrade schema to draft/2020-12 with additionalProperties: false - Add fullsend-check-output after result write - Document refine-escalated control label - Register post-critique-test.sh in Makefile script-test target Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 7:57 PM UTC · Completed 8:07 PM UTC |
fullsend-ai-review
Bot
dismissed stale reviews from themself
Superseded by updated review
fullsend-ai-review
Bot
added
the
requires-manual-review
There was a problem hiding this comment.
Move this to the harness, then env.sandbox.
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
I want to move some vars to the harness file.
2 participants
Summary
create-children.shfor child issue creation (GitHub sub-issues API + Jira hierarchy with fallbacks)comment-helpers.sh,pre-explore.sh, andmarkdown-to-adf.pyRelated Issue
Continuation of agent migration from
konflux-ci/refinementto generic agents repo.Changes
agents/critique.mdharness/critique.yamlenv.runner/env.sandboxformatpolicies/critique.yamlschemas/critique-result.schema.jsondescription_clarityandproject_routingdimensionsscripts/pre-critique.shscripts/post-critique.shscripts/post-critique-test.shscripts/create-children.shenv/critique.envdocs/critique.mdconfig.yamlharness/critique.yamlREADME.mdTesting
bash scripts/post-critique-test.sh— 35/35 tests passingChecklist
PUTverb blocked indisallowedToolsadd_labelhelper consistently (not rawgh api)customized/overlay referencesenv.runner/env.sandboxformatdescription_clarityandproject_routingin critique schemaMade with Cursor