Repository navigation
fix(jira-issue-to-pr): add default tools to spawned conversation - #393
jpelletier1 wants to merge 3 commits into
Conversation
The spawned conversation was created without an explicit tools list,
causing the SDK Agent to default to think+finish only — it had no
terminal or file_editor and could not clone repos, create files, or
run git commands.
Changes:
- Use the 'agent' key (not 'agent_settings') to avoid a double-
registration bug in the agent server
- Explicitly set tools: [{name: terminal}, {name: file_editor}]
- Replace encrypted-settings forwarding with LookupSecret references
for user secrets (consistent with github-repo-monitor and
slack-channel-monitor)
- Forward mcp_config as a top-level field when present
Co-authored-by: openhands <openhands@all-hands.dev>
Without X-Expose-Secrets: plaintext, GET /api/settings returns the LLM api_key as '**********', which the spawned conversation cannot use — causing a LiteLLM 'Missing credentials' error. All other scripts (github-repo-monitor, slack-channel-monitor, github-pr-reviewer) already use this header. Co-authored-by: openhands <openhands@all-hands.dev>
Using X-Expose-Secrets: plaintext puts the raw LLM API key in the network payload. The correct approach (per agent-canvas-environment SKILL.md) is X-Expose-Secrets: encrypted, which returns llm.api_key as a Fernet token that the server decrypts when the conversation is created (secrets_encrypted: True). Also switch back to agent_settings (not agent) and restore the full original payload shape (stuck_detection, autotitle, worktree, initial_message.run) — these were lost in the previous commit. The actual root fix remains unchanged: tools are merged into agent_settings so the spawned agent has terminal + file_editor. Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
1 similar comment
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Reviewed at head aeeb2313050db6772fc946c29da8f246ecdef178 (detached checkout). The CI checks attached to this head are green (test, sync-extensions, sync-sdk-skill, validate-claude-code), uv run --group test pytest tests/ -q passes 442 tests locally, regenerating skills/index.js with node scripts/build-skills-catalog.mjs shows no drift, and the changed script compiles. Merging terminal/file_editor into the tool list and keeping the credential encrypted are the right direction for the reported failure. Two blocking items remain.
1. The PR is not mergeable against current main (blocking). GitHub reports mergeable: false / mergeable_state: dirty, and git merge-tree origin/main HEAD reports content conflicts in skills/index.js and skills/jira-issue-to-pr/scripts/main.py. The main.py conflict comes from main commit 91f27d4 (#449, "require Atlassian Rovo for Jira automation"), which rewrote the same mcp_config hunk this PR edits (main has mcp_config = agent_settings.pop("mcp_config", None) plus payload["mcp_config"] = mcp_config). The branch is based on 20e1b9f while main is now 25377f2, so the green CI above ran against a stale base and does not validate this change against current main. A rebase is required before merge, and it is not purely mechanical: main also moved the automations catalog to automations/catalog/<id>/manifest.json and now requires the atlassian-rovo integration for this automation.
2. Forwarding every configured secret to the spawned conversation is a security problem (blocking). See the inline comment on skills/jira-issue-to-pr/scripts/main.py.
Non-blocking observation. The script still forwards the whole /api/settings agent_settings object, including agent_settings["agent"] from the full-app agent registry. The sibling automations in this repo (skills/github-repo-monitor, skills/slack-channel-monitor, skills/github-pr-reviewer) deliberately build an explicit agent dict instead and document why: forwarding the full-app agent name (e.g. CodeActAgent) "causes a 500 with 'Unknown kind' in production", and the agent_settings code path can double-register the same usage_id in the LLM registry. skills/github-repo-monitor/tests/test_main.py::test_full_app_agent_name_not_forwarded covers this. Since this PR's stated goal is to make the spawned conversation actually start, adopting the sibling pattern (explicit agent with kind: "Agent" plus the tool list) would remove that documented 500 risk instead of layering tools onto the risky path. This behavior is pre-existing rather than introduced here, so it is not blocking on its own.
Note also that the two issue comments already on this PR (from all-hands-bot) ask for the HUMAN: test section of the PR description to be filled in; the description still contains only the unchecked template box.
🔄 CHANGES REQUESTED
| secrets_list = [] | ||
|
|
||
| secrets_payload: dict = {} | ||
| for s in secrets_list: |
There was a problem hiding this comment.
secrets_payload enumerates every secret returned by GET /api/settings/secrets and forwards all of them into the spawned conversation, so the agent receives every configured org secret rather than the ones this task needs.
This is the anti-pattern the repo's own automation guidance warns about. On main, skills/openhands-automation/references/security.md ("Least-Privilege Secrets for Spawned Conversations", added in #429) shows this exact loop under the heading "Anti-pattern: every conversation gets every org secret, needed or not" and states the deployed script "should hardcode the names it needs", because secrets on POST /api/conversations is "a strict allowlist by construction - an omitted name is unreachable". skills/openhands-automation/SKILL.md says the same: "Give spawned conversations only the secrets they need - pass an explicit allowlist, not every configured secret." Neither file is on this branch's base, so the required rebase brings that guidance into scope.
The concrete risk here: this automation's spawned conversation is driven by untrusted Jira ticket text, and summary/description are interpolated straight into the prompt. A task that only needs GITHUB_TOKEN to clone and push therefore also hands the agent every other configured secret (Slack tokens, the Jira API token, automation keys), each resolvable through the per-secret endpoint, and each forwarded entry embeds the automation's own session key. Fix by forwarding an explicit list, e.g. REQUIRED_SECRETS = ["GITHUB_TOKEN"], and building LookupSecret entries only for those names.
There was a problem hiding this comment.
I'm an AI agent (Claude Code, based on Opus 5.5) helping Engel Nyst (@enyst) with project work.
There have been no new commits since all-hands-bot's 2026-09-23 change request, so both of its blocking points (conflicts with main, forwarding every configured secret) still stand.
Main has also absorbed part of this PR since then:
X-Expose-Secrets: encryptedplussecrets_encrypted: Truewere already on the base. The PR only rewords that comment.- Forwarding
mcp_configlanded in #449. - #712 added a separate Cloud/Enterprise dispatch path (
start_cloud_conversation) that this branch doesn't know about.
What's left is the local-path tools fix. I haven't checked whether the current agent server still starts that conversation without tools. If it does, skills/github-issue-to-pr/scripts/main.py on main already combines the explicit-agent and secret-allowlist patterns the bot suggested: kind: "Agent" with terminal/file_editor tools (around L689-697), and an allowlisted AGENT_SECRET_NAMES with _build_secrets_payload (L62, L709). Keep this script's encrypted settings fetch rather than that file's plaintext one.
I suggest closing this PR and opening a small one against current main that applies that pattern to the local path, with evidence from a local run that the spawned conversation can clone and push.
Why
The spawned agent conversation failed in two ways:
toolslist, so the SDK Agent defaulted tothink+finishonly and could not execute bash commands, clone repos, or edit files.GET /api/settings(default) returnsllm.api_keyas**********. The spawned conversation received the placeholder and could not authenticate with the LLM.Summary
X-Expose-Secrets: encryptedto fetch settings — returnsllm.api_keyas a Fernet token; the server decrypts it when the conversation is created (secrets_encrypted: True), so the real API key is never in the network payload.terminalandfile_editorintoagent_settings.toolsexplicitly — without them the SDK Agent defaults to think+finish only.LookupSecretreferences so the spawned agent can clone repos and push branches.mcp_configas a top-level field when present.Issue Number
N/A
How to Test
Deploy the
jira-issue-to-prautomation against a Jira Cloud project with a labeled issue. The spawned conversation should start, authenticate with the LLM, and proceed to clone the target repo and open a PR.Notes
The
encrypted+secrets_encrypted: Truepattern is documented inskills/agent-canvas-environment/SKILL.mdas the correct way to forward LLM credentials without exposing them in plaintext.This PR was created by an AI agent (OpenHands) on behalf of the user.
Jev-Fast-Audit
⚡ Jev fast audit · estimates · 0.30s · commit aeeb231⚠️ reduced context — partial coverage; 6/6 hunks, 3/3 files (missing context: 2).
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage:
All estimates and evidence