From 84a0301415edd6d1385207ec96e4d7f2b6c1fcd9 Mon Sep 17 00:00:00 2001 From: unsupportedpastels Date: Wed, 4 Nov 2026 04:21:40 +0000 Subject: [PATCH] fix(review): the background review never asks the user for a skill's API key The background review thread runs with the finished turn's ContextVars (propagate_context_to_thread), so it inherits the session owner's turn-scoped secret prompt. When the review opened a skill whose required env var is missing (skill_view of the bundled linear skill without LINEAR_API_KEY), the masked "Enter value for ..." panel popped up on the user's screen after the turn had already ended, and the review waited up to the 120s prompt deadline for an answer. The review already resolves dangerous-command approvals to "deny" without asking anyone. Give it the matching secret capture: bind a non-interactive callback for the review fork that reports the setup as skipped. The bound ContextVar wins over both the turn's viewer prompt and the classic CLI's process-global callback, and /refine reviews take the same path. --- agent/background_review.py | 15 +++- .../test_background_review_secret_prompt.py | 76 +++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 tests/agent/test_background_review_secret_prompt.py diff --git a/agent/background_review.py b/agent/background_review.py index 108e4518ed..9215d65d44 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -1069,6 +1069,18 @@ def _bg_review_auto_deny(command, description, **kwargs): return "deny" +def _bg_review_skip_secret(var_name, prompt, metadata=None): + """Non-interactive secret capture: a skill the review opens never asks the user for a key. + + The review thread inherits the turn's ContextVars, including the session owner's turn-scoped + secret prompt, so without this a ``skill_view`` of a skill with a missing required env var would + pop the masked "enter value" panel on the user's screen after the turn already finished. + """ + logger.info("Background review skipped the secret prompt for %s", var_name) + return {"success": True, "reason": "background_review", "stored_as": var_name, "validated": False, + "skipped": True, "message": "Secret setup was skipped."} + + def _set_thread_approval_callback(callback: Any) -> None: from tools.terminal_tool import set_approval_callback @@ -1281,7 +1293,8 @@ def _run_review_in_thread( # driving a Telegram long-poll — for the full duration of the review (tens of seconds), swallowing # their console output (#55769 / #55925). ``thread_scoped_silence`` routes only this thread's writes # to devnull and leaves all other threads on the real streams. - with thread_scoped_silence(): + from tools.skills_tool import bind_turn_secret_capture + with thread_scoped_silence(), bind_turn_secret_capture(_bg_review_skip_secret): _run_review_fork(agent, messages_snapshot, prompt, task_cfg, review_run, st, review_memory, explicit) # A buggy/legacy tool response shape must NOT take down the whole review (the outer # except would discard every action the fork DID complete), so coerce to an empty list. diff --git a/tests/agent/test_background_review_secret_prompt.py b/tests/agent/test_background_review_secret_prompt.py new file mode 100644 index 0000000000..657e93adda --- /dev/null +++ b/tests/agent/test_background_review_secret_prompt.py @@ -0,0 +1,76 @@ +"""The background review never asks the user for a skill's missing API key. + +The review thread inherits the finished turn's ContextVars, including the session owner's +turn-scoped secret prompt. A ``skill_view`` of a skill with a missing required env var inside the +review must resolve to "skipped" instead of opening the masked prompt on the user's screen. +""" + +from __future__ import annotations + +import run_agent as run_agent_module +from run_agent import AIAgent +from tests.agent.test_background_review import ImmediateThread, _bare_agent +from tools import skills_tool +from tools.skills_tool_setup import _capture_required_environment_variables + +_MISSING = [{"name": "LINEAR_API_KEY", "prompt": "Enter value for LINEAR_API_KEY"}] + + +def _viewer_prompt(calls): + def prompt(var_name, prompt, metadata=None): + calls.append(var_name) + return {"success": True, "stored_as": var_name, "validated": False, "skipped": False} + return prompt + + +def _run_review_that_opens_skill(monkeypatch): + """Spawn a real background review whose fork loads a skill with a missing key.""" + results = [] + + class SkillOpeningReviewAgent: + def __init__(self, **kwargs): + self._session_messages = [] + + def run_conversation(self, **kwargs): + results.append(_capture_required_environment_variables("linear", _MISSING)) + + def release_clients(self): + pass + + monkeypatch.setattr(run_agent_module, "AIAgent", SkillOpeningReviewAgent) + monkeypatch.setattr(run_agent_module.threading, "Thread", ImmediateThread) + AIAgent._spawn_background_review( + _bare_agent(), messages_snapshot=[{"role": "user", "content": "hello"}], review_skills=True) + assert len(results) == 1, "the review fork did not run" + return results[0] + + +def test_review_does_not_use_the_turns_secret_prompt(monkeypatch): + calls = [] + with skills_tool.bind_turn_secret_capture(_viewer_prompt(calls)): + result = _run_review_that_opens_skill(monkeypatch) + assert skills_tool._turn_secret_capture.get() is not None # the turn's own binding is intact + + assert calls == [] + assert result["missing_names"] == ["LINEAR_API_KEY"] + assert result["setup_skipped"] is True + + +def test_review_does_not_use_the_cli_secret_prompt(monkeypatch): + calls = [] + monkeypatch.setattr(skills_tool, "_secret_capture_callback", _viewer_prompt(calls)) + + result = _run_review_that_opens_skill(monkeypatch) + + assert calls == [] + assert result["setup_skipped"] is True + + +def test_live_turn_still_prompts_for_the_key(monkeypatch): + """Control: outside the review the same skill load still reaches the viewer's prompt.""" + calls = [] + with skills_tool.bind_turn_secret_capture(_viewer_prompt(calls)): + result = _capture_required_environment_variables("linear", _MISSING) + + assert calls == ["LINEAR_API_KEY"] + assert result["missing_names"] == []