feat: add generic explore agent - #11
Conversation
PR Summary by QodoAdd generic explore agent for issue research (GitHub/Jira)
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
|
🤖 Finished Review · ✅ Success · Started 8:29 PM UTC · Completed 8:39 PM UTC |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsHigh
Medium
Low8 low-severity findings
Next steps:
Previous runReviewFindingsHigh
Medium
Low9 low-severity findings
Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low10 low-severity findings
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low6 low-severity findings
Next steps:
Previous run (4)ReviewFindingsHigh
Medium
Low5 low-severity findings
Labels: PR adds new agent with sandbox credential handling and has security-relevant findings Next steps:
Previous run (5)ReviewFindingsHigh
Medium
Low5 low-severity findings
Next steps:
Previous run (6)ReviewFindingsHigh
Medium
Low6 low-severity findings
Next steps:
Previous run (7)ReviewFindingsHigh
Medium
Low12 low-severity findings
Next steps:
Previous run (8)ReviewFindingsHigh
Medium
Low13 low-severity findings
Next steps:
Previous run (9)ReviewFindingsHigh
Medium
Low12 low-severity findings
Previous run (10)ReviewVerdict: approve All 14 findings from the prior review have been addressed in commit db4947e. The fixes are clean and complete:
Re-review assessmentSix review dimensions were evaluated against the current HEAD:
The code is clean, well-organized, and ready to merge.
Previous runReviewVerdict: request-changes This PR adds a well-structured exploration agent that follows the existing triage agent's architecture. The credential isolation (keeping Jira/GitHub tokens out of the sandbox), input validation, and repo cloning safeguards are well done. However, there are several issues that should be addressed before merging — one logic error that silently breaks gap reporting in posted comments, schema/prompt mismatches that will cause downstream confusion, and security defense-in-depth gaps. FindingsHigh
Medium
Labels: PR adds a new explore agent with harness, scripts, schema, and skills. |
![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.
|
Additional review items from the summary (no inline thread) addressed in
All 14 inline review threads have individual replies explaining the fix. |
|
🤖 Finished Review · ✅ Success · Started 8:48 PM UTC · Completed 9:00 PM UTC |
fullsend-ai-review
Bot
added
the
requires-manual-review
- Block all gh api calls in sandbox (match fix agent pattern) - Remove FULLSEND_OUTPUT_FILE override and GH_TOKEN from sandbox - Guard comment-helpers.sh dependency with clear PR #11 message - Sanitize GHA workflow command output to prevent injection - Check label mutation success before logging success notices - Document proposed_description body updates in user docs - Remove missing icon reference from docs Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- 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>
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
I think this needs some changes before we can merge. See inline comments.
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| source "${SCRIPT_DIR}/comment-helpers.sh" | ||
|
|
There was a problem hiding this comment.
[critical] We ran into this exact problem with the prioritize agent (PR #35, cac8127): base composition fetches scripts as individual content-addressed blobs without their sibling directories. source "${SCRIPT_DIR}/comment-helpers.sh" will break under base composition because comment-helpers.sh won't exist at SCRIPT_DIR.
Same problem applies to python3 "${SCRIPT_DIR}/adf-to-markdown.py" further down.
The fix for prioritize was to inline the shared functions. That's not great for code reuse, but it's how the platform works today. Worth thinking about whether there's a better resolution mechanism we could pursue, but in the meantime these scripts need to be self-contained.
There was a problem hiding this comment.
Addressed in 6d6dcb2 for the install-overlay pattern we need for konflux-ci/refinement:
Added _resolve_companion so companions are loaded from:
SCRIPT_DIR(normal checkout / vendored install scripts)$GITHUB_WORKSPACE/.fullsend/scriptsor$FULLSEND_DIR/scripts(install overlay when the harnessbase:blob has no siblings)
Refinement (and similar installs) override pre_script/post_script locally and ship companions under .fullsend/scripts/, so base: composition works for the consumer path we’re validating now.
Full inlining of comment-helpers.sh / ADF converters (prioritize cac8127 style) remains the most durable fix for pure base: consumers that neither override scripts nor vendor companions — happy to do that as a follow-up if you want it before merge. This change unblocks verified testing of agent prompts via base: from refinement.
| local label="$1" | ||
| if [[ ! "${label}" =~ ^[a-zA-Z0-9._/:\ +\-]+$ ]]; then | ||
| echo "::warning::Refused pipeline label '${label}' -- contains invalid characters" | ||
| return 1 |
There was a problem hiding this comment.
[critical] Same base-composition issue as pre-explore.sh — source "${SCRIPT_DIR}/comment-helpers.sh" won't resolve when the harness is consumed via base: URL. See the inline comment on pre-explore.sh:27 for context.
There was a problem hiding this comment.
Same fix as pre-explore: _resolve_companion in 6d6dcb2 for comment-helpers.sh / ADF converters under install .fullsend/scripts when the post-script is fetched as an isolated base: blob. See reply on pre-explore.sh for the full rationale and follow-up (full inline) option.
| { | ||
| "$schema": "https://json-schema.org/draft/2020-12/schema", | ||
| "$id": "explore-result.schema.json", | ||
| "title": "Exploration Result", |
There was a problem hiding this comment.
[important] The schema is quite a bit more permissive than the prompt implies. A few things I noticed:
data_sources— the prompt says "Important: Include thedata_sourcesfield" but the schema doesn't require it- The individual confidence dimensions (
technical_landscape,related_work, etc.) are optional in the schema even though the prompt presents them as a mandatory table - No
additionalProperties: falseanywhere — the existingtriage-result.schema.jsonuses it at the top level
If the validation loop is supposed to catch malformed output, the schema needs to match what the prompt actually asks for.
| curl -sSf -X PUT \ | ||
| -H "Authorization: Basic $AUTH" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "{\"update\":{\"labels\":[{\"add\":\"${SIGNAL_LABEL}\"}]}}" \ |
There was a problem hiding this comment.
[important] SIGNAL_LABEL is interpolated directly into JSON via shell string interpolation here. Even with validate_label_name, the regex allows spaces and backslashes which could cause JSON parsing issues. Using jq -n --arg label "$SIGNAL_LABEL" '...' to construct the payload (like pre-explore.sh does elsewhere) would eliminate the injection surface.
|
|
||
| _redact_secrets() { | ||
| if command -v fullsend >/dev/null 2>&1; then | ||
| fullsend scan output |
There was a problem hiding this comment.
[important] Two things here:
- Does
fullsend scan outputactually read from stdin? The subcommand name suggests it might scan a directory called "output" rather than reading a pipe. If so, the body gets silently lost. - The fallback (
cat) posts content with no secret scanning at all. If fullsend isn't on PATH in a misconfigured environment, secrets in the comment body go straight to Jira/GitHub.
Would it be safer to refuse to post rather than post unscanned?
| """Convert a full ADF document to Markdown.""" | ||
| if not isinstance(adf, dict): | ||
| return str(adf) if adf else "" | ||
|
|
There was a problem hiding this comment.
[minor] '""' appears twice in this condition — looks like a copy-paste duplicate. Was the second one meant to be a different value?
| validate_repo() { | ||
| local ref="$1" | ||
| local http_code | ||
| http_code=$(GIT_TERMINAL_PROMPT=0 curl -sf -o /dev/null -w "%{http_code}" \ |
There was a problem hiding this comment.
[minor] lang_ext is set here and updated based on detected language, but never referenced after this block. The find on line 437 always searches all extensions regardless. Dead code.
| source "${SCRIPT_DIR}/comment-helpers.sh" | ||
|
|
||
| echo "::notice::Pre-explore: fetching issue data (source=${ISSUE_SOURCE}, key=${ISSUE_KEY})" | ||
|
|
There was a problem hiding this comment.
[minor] (non-blocking) Under set -u, if ISSUE_SOURCE or ISSUE_KEY is unset, this line aborts with a cryptic "unbound variable" error before the helpful validation messages on lines 32-42 can run. Using ${ISSUE_SOURCE:-} here or moving the notice after validation would give better error messages.
| }' > "$WORKSPACE/issue-context.json" | ||
|
|
||
| elif [[ "${ISSUE_SOURCE}" == "github" ]]; then | ||
| if [[ -z "${REPO_FULL_NAME:-}" ]]; then |
There was a problem hiding this comment.
[minor] (non-blocking) This line uses "${GITHUB_ENV}" without the :-/dev/null fallback that lines 319 and 505 use. Running outside GitHub Actions would fail here.
| - skills/jira-routing # team-specific | ||
| env: | ||
| runner: | ||
| EXPLORE_READY_LABEL: ready-to-refine |
There was a problem hiding this comment.
[nit] (non-blocking) The example label ready-to-refine bakes in a pipeline-specific name. Since the docs emphasize labels are generic and opt-in, something like explore-complete would reinforce that.
Update for konflux-ci/refinement
|
|
🤖 Review · |
6d6dcb2 to
00c3629
Compare
CI fix pass (
|
|
🤖 Review · |
Introduce explore harness, agent prompt, scripts, schema, and skills. Consume optional ORG_KNOWLEDGE for install-injected org context without failing when the pack is absent. Resolve companion script helpers for base-composition installs. Fix shellcheck findings in pre-explore and comment helpers. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
00c3629 to
626639a
Compare
|
Rebased onto latest |
|
🤖 Finished Review · ✅ Success · Started 8:13 PM UTC · Completed 8:32 PM UTC |
|
🤖 Review · |
Hints file is written to /tmp/workspace by pre-explore; referencing it
via ${JIRA_API_HINTS} failed ValidateRunnerEnv before the pre-script ran.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 1:25 AM UTC · Completed 1:38 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Phase 0 duplicate gate blocks exploration before deep work. Sticky warns and skips ready-to-refine; a second /fs-explore (marker present) overrides. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 11:49 AM UTC · Completed 12:10 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Newer fullsend rejects absolute host_files.src on URL-composed bases. Runtime workspace mounts belong on the install overlay. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 8:19 PM UTC · Completed 8:37 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Phase 0 cats /tmp/workspace/duplicate-gate.json; also try the sandbox mount path and report clearly when neither exists so override=true is not silently missed. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Score confidence 0.0–5.0 (one decimal). Rename shaped/output issue identifiers from key→issue_id and project.key→project.id so fullsend SecretRedactor does not redact Jira tickets (field names containing "key" match the secret pattern). Assisted-by: Cursor Signed-off-by: Adam Scerra <ascerra@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:03 PM UTC · Completed 5:22 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…s-info Score alone no longer forces needs-info when the agent assessed disposition as complete. The post-script now checks disposition first: needs_info always gets the needs-info label, complete always gets ready-to-refine, and the score threshold is a backward-compat fallback for old results without an explicit disposition. Adds needs_info disposition enum value, open_questions array (with blocking flag to distinguish user-facing vs implementation-facing gaps) to the schema and agent prompt. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 10:21 PM UTC · Completed 10:40 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Preserve CI-prefetched clones in CLONED/manifest (previously skipped and dropped), authenticate validate_repo + git clone with GH_TOKEN, and teach the agent to use authenticated gh api instead of anonymous curl (60/hr). Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · Commit: |
Use http.extraHeader for authenticated clones instead of x-access-token in the URL (process-list leak). Point related-work search at gh search, and correct docs that wrongly claimed tokens never enter the sandbox. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 11:08 PM UTC · Completed 11:24 PM UTC Commit: |
| '{"issue_id": $k, "summary": $s, "description": $d}') | ||
| fi | ||
|
|
||
| CHILDREN_JSON=$(jira_get "${JIRA_BASE}/search?jql=parent=${ISSUE_KEY}&fields=summary,status,issuetype&maxResults=50" 2>/dev/null \ |
There was a problem hiding this comment.
[high] logic-error
The pre-script fetches Jira children (line 172) and team usage (line 253) via ${JIRA_BASE}/search?jql= which resolves to /rest/api/3/search?jql=. The script's own jira-api-hints.json documents that /rest/api/3/search returns HTTP 410. Both calls fail silently due to error swallowing, delivering empty children and team_usage to the agent.
Suggested fix: Replace both occurrences of ${JIRA_BASE}/search?jql= with GET requests to ${JIRA_BASE}/search/jql using --get --data-urlencode query parameters, matching the pattern in the jira-api-hints probe section.
|
|
||
| ## What the agent CANNOT do | ||
|
|
||
| - The agent cannot call the Jira API directly (sandbox network policy blocks it) |
There was a problem hiding this comment.
[medium] logic-error
SKILL.md states 'The agent cannot call the Jira API directly (sandbox network policy blocks it)' but the network policy allows *.atlassian.net:443, the harness passes Jira credentials to the sandbox, and the agent prompt instructs curl+Basic auth usage. docs/explore.md was updated but SKILL.md was not.
Suggested fix: Update the 'What the agent CANNOT do' section to state the agent CAN make read-only Jira API calls using injected credentials.
| { | ||
| "source": "jira", | ||
| "host": "your-org.atlassian.net", | ||
| "key": "PROJ-1620", |
There was a problem hiding this comment.
[low] logic-error
SKILL.md documents field name 'key' for issue identifiers and 'project.key', but the pre-script writes 'issue_id' and 'project.id'. The agent would use wrong field names.
Suggested fix: Update the SKILL.md JSON example to use 'issue_id' instead of 'key' throughout, and 'id' instead of 'key' for the project object.
| if [[ -n "${GITHUB_ISSUE_NUMBER:-}" && "${GITHUB_ISSUE_NUMBER}" != "N/A" ]]; then | ||
| EXISTING_GH_LABELS=$(gh api "repos/${REPO_FULL_NAME}/labels" --paginate --jq '.[].name' 2>/dev/null || true) | ||
| if github_label_exists "$SIGNAL_LABEL"; then | ||
| gh api "repos/${REPO_FULL_NAME}/issues/${GITHUB_ISSUE_NUMBER}/labels" \ |
There was a problem hiding this comment.
[low] logic-error
On re-runs where confidence crosses the threshold, the previously-applied pipeline label is never removed. Both labels could be present simultaneously.
Suggested fix: Before adding SIGNAL_LABEL, remove the opposite label.
| # shellcheck disable=SC1090 | ||
| source "$(_resolve_companion comment-helpers.sh)" | ||
|
|
||
| echo "::notice::Pre-explore: fetching issue data (source=${ISSUE_SOURCE}, key=${ISSUE_KEY})" |
There was a problem hiding this comment.
[low] GHA-workflow-command-injection
ISSUE_SOURCE and ISSUE_KEY interpolated into ::notice:: BEFORE input validation at lines 104-117. Validation-after-emission is a defense-in-depth gap.
Suggested fix: Move the ::notice:: to after the input validation block.
| if [[ -f "$meta" ]] && command -v jq >/dev/null 2>&1 && command -v curl >/dev/null 2>&1; then | ||
| local origin base_url tmp | ||
| origin=$(jq -r '.url // empty' "$meta" 2>/dev/null || true) | ||
| if [[ -n "$origin" && "$origin" == http*://* ]]; then |
There was a problem hiding this comment.
[low] remote-code-fetch
The _resolve_companion function fetches and executes remote shell scripts based on a URL read from metadata.json. The origin URL is validated only with a loose prefix check.
| if command -v fullsend >/dev/null 2>&1; then | ||
| fullsend scan output | ||
| else | ||
| echo "::warning::fullsend not on PATH — posting comment without secret scanning" >&2 |
There was a problem hiding this comment.
[low] secret-exposure
When fullsend CLI is not on PATH, _redact_secrets falls back to cat, posting comment content without secret scanning.
| md_rows.append("| " + " | ".join(cell_texts) + " |") | ||
|
|
||
| if len(md_rows) >= 1: | ||
| col_count = md_rows[0].count("|") - 1 |
There was a problem hiding this comment.
[low] edge-case
render_table calculates separator column count by counting | in the first rendered row. Literal | in cell content inflates the count.
|
|
||
|
|
||
| def render_marks(text: str, marks: list) -> str: | ||
| """Wrap text with Markdown formatting based on ADF marks.""" |
There was a problem hiding this comment.
[low] edge-case
render_marks applies marks in input array order. If code precedes strong, bold asterisks render literally inside the code span.
Closing without merge — keep the branchClosing #11 (explore). Explore is not cancelled. This draft PR was kept open so installs could pin WIP SHAs while we iterated. Every push re-triggered the fullsend review agent. We looked at extending Team decision 2026-08-10 (notes). Work continues on
|
|
🤖 Finished Retro · ✅ Success · Started 12:41 AM UTC · Completed 12:56 AM UTC Commit: |
Retro: PR #11 — feat: add generic explore agentTimeline
Review quality analysisWhat the review bot did well: Found 7 actionable bugs in its first cycle — What the human caught that the bot missed (across all 11 cycles):
The bot later picked up 2 findings that overlapped with the human's (SIGNAL_LABEL JSON injection and Existing issues providing coverageMost improvement areas from this PR are already tracked:
Proposals filed |
2 participants
Summary
ISSUE_SOURCE)EXPLORE_READY_LABEL/EXPLORE_NEEDS_INFO_LABELenv vars (no hardcoded refinement labels)public-researchandjira-readskills for downstream customization via harnessbase:compositionFiles added
agents/explore.mdharness/explore.yamlpolicies/explore.yamlschemas/explore-result.schema.jsonscripts/pre-explore.shscripts/post-explore.shscripts/comment-helpers.shscripts/adf-to-markdown.py,markdown-to-adf.pyskills/public-research,skills/jira-readSecurity review
ISSUE_SOURCE,ISSUE_KEY,REPO_FULL_NAME, and pipeline label namesfullsend scanruns before posting commentsCode review fixes applied
REFERENCED_REPOS_DIRto sandbox viaenv/explore.envREPO_FULL_NAMEformat in pre-scriptGITHUB_ISSUE_NUMBERfrom pre-script for GitHub runsFollow-up (separate PRs)
base:URL +jira-routingskill + Konflux pipeline labelsTest plan
fullsend agent addMade with Cursor