diff --git a/agents/test-plan-generator/hackbot_agents/test_plan_generator/__main__.py b/agents/test-plan-generator/hackbot_agents/test_plan_generator/__main__.py index aaae133ca7..68a11794e3 100644 --- a/agents/test-plan-generator/hackbot_agents/test_plan_generator/__main__.py +++ b/agents/test-plan-generator/hackbot_agents/test_plan_generator/__main__.py @@ -1,5 +1,4 @@ from hackbot_runtime import HackbotContext, run_async -from hackbot_runtime.actions.testrail import record_test_plan from pydantic_settings import BaseSettings, SettingsConfigDict from .agent import TestPlanGeneratorResult, run_test_plan_generator @@ -32,8 +31,8 @@ async def main(ctx: HackbotContext) -> TestPlanGeneratorResult: firefox_path=firefox_path, log=ctx.log_path, verbose=True, + actions_recorder=ctx.actions, ) - record_test_plan(ctx.actions, result.result.model_dump(mode="json")) return result diff --git a/agents/test-plan-generator/hackbot_agents/test_plan_generator/agent.py b/agents/test-plan-generator/hackbot_agents/test_plan_generator/agent.py index 08122e2495..3eaddcf70e 100644 --- a/agents/test-plan-generator/hackbot_agents/test_plan_generator/agent.py +++ b/agents/test-plan-generator/hackbot_agents/test_plan_generator/agent.py @@ -11,18 +11,14 @@ McpServerConfig, ResultMessage, ) -from hackbot_runtime import AgentError, HackbotAgentResult +from hackbot_runtime import ActionsRecorder, AgentError, HackbotAgentResult +from hackbot_runtime.actions import ACTIONS_SERVER_NAME +from hackbot_runtime.actions.claude_sdk import actions_server_for, actions_to_tool_names +from hackbot_runtime.actions.testrail import ACTION_TYPE as TESTRAIL_SUBMIT_TEST_PLAN from hackbot_runtime.claude import Reporter -from .config import DEVTOOLS_TOOLS +from .config import DEVTOOLS_TOOLS, ENABLED_ACTION_TYPES from .devtools_mcp import build_devtools_server -from .result import ( - RESULT_SERVER_NAME, - SUBMIT_RESULT_TOOL, - ResultCollector, - TestPlanResult, - build_result_server, -) HERE = Path(__file__).resolve().parent @@ -30,7 +26,7 @@ class TestPlanGeneratorResult(HackbotAgentResult): - result: TestPlanResult | None = None + result: str | None = None def load_system_prompt() -> str: @@ -45,12 +41,12 @@ def build_user_prompt( f"Feature name:\n{feature_name}\n\n" f"Feature description:\n{feature_description}\n\n" f"Test scope:\n{test_scope}\n\n" - "Use the provided feature name as the structured result feature. " + "Use the provided feature name as the TestRail action feature. " "Keep all generated test cases within the provided test scope.\n\n" "Follow the required workflow exactly: before execution, generate no more than 30 " "test cases to cover all distinct behaviors, meaningful variations, and " "negative scenarios. Run the cases in order, stop each case after its first " - "failed step, and submit exactly one structured result." + "failed step, and record exactly one TestRail test plan action." ) @@ -65,6 +61,7 @@ async def run_test_plan_generator( firefox_path: str | None = None, verbose: bool = False, log: Path | None = None, + actions_recorder: ActionsRecorder | None = None, ) -> TestPlanGeneratorResult: """Generate and run a Firefox QA test plan for one feature.""" subject = feature_name @@ -76,12 +73,14 @@ async def run_test_plan_generator( enable_script=True, ) - result_collector = ResultCollector() - result_server = build_result_server(result_collector) + actions_recorder, actions_server = actions_server_for( + actions_recorder, types=ENABLED_ACTION_TYPES + ) + enabled_action_tools = actions_to_tool_names(ENABLED_ACTION_TYPES) mcp_servers: dict[str, McpServerConfig] = { "firefox-devtools": devtools_server, - RESULT_SERVER_NAME: result_server, + ACTIONS_SERVER_NAME: actions_server, } options = ClaudeAgentOptions( @@ -90,7 +89,7 @@ async def run_test_plan_generator( permission_mode="bypassPermissions", allowed_tools=[ *DEVTOOLS_TOOLS, - SUBMIT_RESULT_TOOL, + *enabled_action_tools, ], model=model, max_turns=max_turns, @@ -118,13 +117,16 @@ async def run_test_plan_generator( f"{subject} test-plan generation failed: " f"{result_msg.result or result_msg.subtype}" ) - if result_collector.result is None: + if not any( + action["type"] == TESTRAIL_SUBMIT_TEST_PLAN + for action in actions_recorder.actions + ): raise AgentError( - f"{subject}: agent finished without submitting a result via submit_result" + f"{subject}: agent finished without recording a TestRail test plan action" ) return TestPlanGeneratorResult( - result=result_collector.result, + result=result_msg.result, num_turns=result_msg.num_turns, total_cost_usd=result_msg.total_cost_usd, ) diff --git a/agents/test-plan-generator/hackbot_agents/test_plan_generator/config.py b/agents/test-plan-generator/hackbot_agents/test_plan_generator/config.py index 3760e2ff42..b943d9ef01 100644 --- a/agents/test-plan-generator/hackbot_agents/test_plan_generator/config.py +++ b/agents/test-plan-generator/hackbot_agents/test_plan_generator/config.py @@ -1,3 +1,9 @@ +# Recordable action types the agent may take, by dotted id. It generates and runs +# test cases, then records them for TestRail; it takes no other action. +ENABLED_ACTION_TYPES = [ + "testrail.submit_test_plan", +] + # Firefox DevTools MCP tools (@mozilla/firefox-devtools-mcp-moz), exposed under # the "firefox-devtools" server name. Keep this focused on tools needed to # generate and execute Firefox QA cases: page interaction, observation, diff --git a/agents/test-plan-generator/hackbot_agents/test_plan_generator/prompts/system.md b/agents/test-plan-generator/hackbot_agents/test_plan_generator/prompts/system.md index bb508182b3..bab8eee10f 100644 --- a/agents/test-plan-generator/hackbot_agents/test_plan_generator/prompts/system.md +++ b/agents/test-plan-generator/hackbot_agents/test_plan_generator/prompts/system.md @@ -2,7 +2,8 @@ You are a Firefox QA test-plan generation and execution agent. Generate test cases from the provided Firefox feature name, feature description, and test scope, run them in Firefox with the available DevTools MCP tools, and -report only pass/fail/unsuitable results. Do not try to fix, patch or make changes. +record the generated test plan for TestRail. Do not try to fix, patch or make +changes. ## Required workflow @@ -10,19 +11,18 @@ report only pass/fail/unsuitable results. Do not try to fix, patch or make chang variations, and negative scenarios before running any case. 2. Each test case must have: - A title. - - A primary execution context label: `chrome` or `content`. - Ordered test steps, each with an `action` and optional `expectation`. 3. Run the generated cases and steps in order. -4. Submit one final structured result with `submit_result`. - - Use the provided feature name as the structured result feature. +4. Record one final TestRail action with `testrail_submit_test_plan`. + - Use the provided feature name as the action feature. ## Context guidance -Choose a primary context label per case: `content` for normal web page or -document behavior; `chrome` for Firefox UI, browser state, preferences, toolbar, -menus, panels, downloads, history, bookmarks, PDF viewer chrome behavior, or -uncertainty. The label describes what the case mainly exercises; it does not -restrict per-step tool choice. +Decide which context each case mainly exercises and pick tools accordingly: +`content` for normal web page or document behavior; `chrome` for Firefox UI, +browser state, preferences, toolbar, menus, panels, downloads, history, +bookmarks, PDF viewer chrome behavior, or uncertainty. This judgment guides your +tool selection and it does not restrict per-step tool choice. Use the most appropriate DevTools MCP tool for each step. Prefer content tools for page/DOM interaction and privileged-context tools for browser UI/state or @@ -68,9 +68,8 @@ Mark a case as `unsuitable` only if it requires: ## Reporting -The final answer must be submitted through `submit_result` exactly once. A prose -message is not enough. Include one case result for every generated test case. +Record the generated test plan through `testrail_submit_test_plan` exactly once. +A prose message is not enough. -For failed steps, set `failure_reason` to a short explanation of the observed -failure. For failed or unsuitable cases, set the case-level `failure_reason` as -well. Leave `failure_reason` empty for passed steps and passed cases. +Then close with a write-up of the execution: which cases passed, failed, or were +unsuitable, with concise observations for the failed and unsuitable ones. diff --git a/agents/test-plan-generator/hackbot_agents/test_plan_generator/result.py b/agents/test-plan-generator/hackbot_agents/test_plan_generator/result.py deleted file mode 100644 index 674650b5a4..0000000000 --- a/agents/test-plan-generator/hackbot_agents/test_plan_generator/result.py +++ /dev/null @@ -1,154 +0,0 @@ -"""Structured result reporting for the test-plan-generator agent.""" - -from __future__ import annotations - -from typing import Literal - -from claude_agent_sdk import McpServerConfig, create_sdk_mcp_server, tool -from pydantic import ( - BaseModel, - Field, - ValidationError, - model_validator, -) - -RESULT_SERVER_NAME = "test-plan-generator" -SUBMIT_RESULT_TOOL = f"mcp__{RESULT_SERVER_NAME}__submit_result" - - -class GeneratedTestStep(BaseModel): - action: str = Field(description="The action a QA engineer should perform.") - expectation: str | None = Field( - default=None, - description=("Expected result for this step."), - ) - - -class GeneratedTestCase(BaseModel): - id: int = Field(description="Sequential case id starting at 1.") - title: str - context: Literal["chrome", "content"] = Field( - description=( - "Primary context label for what the case mainly exercises. This is " - "guidance for tool selection, not a restriction on which available " - "DevTools MCP tools may be used for individual steps." - ) - ) - preconditions: str | None = None - steps: list[GeneratedTestStep] = Field( - description=( - "Concise ordered test steps. Each step has an action and an optional " - "expectation." - ) - ) - - @model_validator(mode="after") - def _validate_steps(self) -> "GeneratedTestCase": - if not self.steps: - raise ValueError("each generated test case must have at least one step") - if not any(step.expectation for step in self.steps): - raise ValueError( - "each generated test case must include an expected result on at " - "least one verification step" - ) - return self - - -class StepResult(BaseModel): - step_number: int - status: Literal["passed", "failed", "not_run"] - observation: str - failure_reason: str | None = Field( - default=None, - description=( - "Required when status is failed. A concise reason why the step failed, " - "based only on what was observed during execution." - ), - ) - - @model_validator(mode="after") - def _validate_failure_reason(self) -> "StepResult": - if self.status == "failed" and not self.failure_reason: - raise ValueError("failed steps must include failure_reason") - return self - - -class TestCaseResult(BaseModel): - id: int - status: Literal["passed", "failed", "unsuitable"] - step_results: list[StepResult] - summary: str - failure_reason: str | None = Field( - default=None, - description=( - "Required when status is failed or unsuitable. A concise reason why " - "the case failed or could not be run, useful for later developer review." - ), - ) - - @model_validator(mode="after") - def _validate_failure_reason(self) -> "TestCaseResult": - if self.status in {"failed", "unsuitable"} and not self.failure_reason: - raise ValueError("failed or unsuitable cases must include failure_reason") - return self - - -class TestPlanResult(BaseModel): - feature: str - generated_test_cases: list[GeneratedTestCase] - results: list[TestCaseResult] - summary: str - - @model_validator(mode="after") - def _validate_result(self) -> "TestPlanResult": - case_count = len(self.generated_test_cases) - if not 1 <= case_count <= 30: - raise ValueError("generated_test_cases must contain 1 to 30 cases") - - case_ids = [case.id for case in self.generated_test_cases] - expected_ids = list(range(1, case_count + 1)) - if case_ids != expected_ids: - raise ValueError("generated test case ids must be sequential starting at 1") - - result_ids = [result.id for result in self.results] - if result_ids != expected_ids: - raise ValueError( - "results must contain one result for each generated test case id" - ) - - return self - - -SUBMIT_RESULT_SCHEMA = { - **TestPlanResult.model_json_schema(), - "additionalProperties": False, -} - - -class ResultCollector: - """Holds the result submitted by the agent, if any.""" - - def __init__(self) -> None: - self.result: TestPlanResult | None = None - - -def build_result_server(collector: ResultCollector) -> McpServerConfig: - """Build an in-process MCP server exposing the ``submit_result`` tool.""" - - @tool( - "submit_result", - "Submit the final generated Firefox QA test plan and execution result. " - "Call exactly once, after all generated test cases have been run.", - SUBMIT_RESULT_SCHEMA, - ) - async def submit_result(args: dict) -> dict: - try: - collector.result = TestPlanResult.model_validate(args) - except ValidationError as exc: - return { - "content": [{"type": "text", "text": f"Invalid result: {exc}"}], - "is_error": True, - } - return {"content": [{"type": "text", "text": "Result recorded."}]} - - return create_sdk_mcp_server(name=RESULT_SERVER_NAME, tools=[submit_result]) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py b/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py index d009dd2974..e429dd229c 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py @@ -1,9 +1,4 @@ -"""TestRail-domain recordable actions. - -The test plan generator records this action deterministically after its -structured result has been validated. The external TestRail mutation still -happens only in the apply side handler. -""" +"""TestRail recordable actions.""" from __future__ import annotations @@ -34,9 +29,6 @@ class TestRailStepInput(BaseModel): class TestRailCaseInput(BaseModel): id: int title: str = Field(description="TestRail test case title.") - context: str | None = Field( - default=None, description="Optional context for where this case applies." - ) preconditions: str | None = Field( default=None, description="Optional setup required before running this case." ) @@ -75,7 +67,9 @@ def expectations_must_include_verification(self) -> "TestRailCaseInput": class SubmitTestPlanInput(BaseModel): feature: str = Field(description="Feature covered by the generated test cases.") generated_test_cases: list[TestRailCaseInput] = Field( - min_length=1, description="Generated test cases to upload to TestRail." + min_length=1, + max_length=30, + description="Generated test cases to upload to TestRail.", ) @field_validator("feature") @@ -86,6 +80,14 @@ def feature_must_not_be_blank(cls, value: str) -> str: raise ValueError("feature must not be blank") return value + @model_validator(mode="after") + def case_ids_must_be_sequential(self) -> "SubmitTestPlanInput": + case_ids = [case.id for case in self.generated_test_cases] + expected_ids = list(range(1, len(self.generated_test_cases) + 1)) + if case_ids != expected_ids: + raise ToolError("test case ids must be sequential starting at 1") + return self + def _confirm(recorder: ActionsRecorder, action_type: str) -> str: return f"Recorded {action_type} (#{len(recorder.actions) - 1})." @@ -118,30 +120,30 @@ async def submit_test_plan( ], generated_test_cases: Annotated[ list[TestRailCaseInput], - Field(description="Generated test cases to upload together to TestRail."), + # Bounds are repeated from SubmitTestPlanInput: this copy is the agent's + # schema, that one is the enforcement. + Field( + min_length=1, + max_length=30, + description="Generated test cases to upload together to TestRail.", + ), ], ) -> str: """Record a generated test plan for deferred TestRail submission. - This records one reviewed action. The apply step creates a new TestRail - suite, creates a section in it, and uploads all supplied test cases. - Nothing is sent to TestRail during the agent run. + Call this at most once per run: the apply step creates a new TestRail suite + for every recorded action, so a second call would duplicate the whole plan. + The apply step creates the suite, creates a section in it, and uploads all + supplied test cases. Nothing is sent to TestRail during the agent run. """ + if any(action["type"] == ACTION_TYPE for action in recorder.actions): + raise ToolError( + "a test plan is already recorded for this run; do not call " + "submit_test_plan again" + ) params = _validated_params(feature, generated_test_cases) recorder.record(ACTION_TYPE, params) return _confirm(recorder, ACTION_TYPE) -def record_test_plan( - recorder: ActionsRecorder, - test_plan: dict[str, Any], -) -> dict: - """Record validated generated cases for deferred TestRail submission.""" - params = _validated_params( - str(test_plan.get("feature") or ""), - list(test_plan.get("generated_test_cases") or []), - ) - return recorder.record(ACTION_TYPE, params) - - TOOLS = tools_in(__name__) diff --git a/libs/hackbot-runtime/tests/test_testrail_action.py b/libs/hackbot-runtime/tests/test_testrail_action.py index 75fce89617..4c17ed077c 100644 --- a/libs/hackbot-runtime/tests/test_testrail_action.py +++ b/libs/hackbot-runtime/tests/test_testrail_action.py @@ -11,7 +11,6 @@ def _cases(): { "id": 1, "title": "The PDF opens", - "context": "content", "preconditions": "A PDF is available.", "steps": [ {"action": "Open the PDF", "expectation": "The PDF is displayed."} @@ -31,7 +30,16 @@ async def test_submit_test_plan_tool_records_deferred_action(): assert recorder.actions[0]["type"] == ACTION_TYPE assert recorder.actions[0]["params"] == { "feature": "Feature", - "generated_test_cases": _cases(), + "generated_test_cases": [ + { + "id": 1, + "title": "The PDF opens", + "preconditions": "A PDF is available.", + "steps": [ + {"action": "Open the PDF", "expectation": "The PDF is displayed."} + ], + } + ], } @@ -42,7 +50,13 @@ async def test_submit_test_plan_tool_rejects_invalid_input(): await testrail.submit_test_plan( recorder, feature=" ", - generated_test_cases=[{"id": 1, "title": "Case", "steps": []}], + generated_test_cases=[ + { + "id": 1, + "title": "Case", + "steps": [], + } + ], ) assert "invalid TestRail submission" in str(exc.value) @@ -69,6 +83,53 @@ async def test_submit_test_plan_tool_rejects_cases_without_expectation(): assert recorder.actions == [] +async def test_submit_test_plan_tool_rejects_non_sequential_case_ids(): + recorder = ActionsRecorder() + cases = _cases() + cases[0]["id"] = 2 + + with pytest.raises(ToolError) as exc: + await testrail.submit_test_plan( + recorder, + feature="Feature", + generated_test_cases=cases, + ) + + assert "test case ids must be sequential starting at 1" in str(exc.value) + assert recorder.actions == [] + + +async def test_submit_test_plan_tool_rejects_a_second_submission(): + recorder = ActionsRecorder() + await testrail.submit_test_plan( + recorder, feature="Feature", generated_test_cases=_cases() + ) + + with pytest.raises(ToolError) as exc: + await testrail.submit_test_plan( + recorder, feature="Other feature", generated_test_cases=_cases() + ) + + assert "already recorded" in str(exc.value) + assert [action["params"]["feature"] for action in recorder.actions] == ["Feature"] + + +async def test_submit_test_plan_tool_rejects_more_than_thirty_cases(): + recorder = ActionsRecorder() + case = _cases()[0] + cases = [{**case, "id": index} for index in range(1, 32)] + + with pytest.raises(ToolError) as exc: + await testrail.submit_test_plan( + recorder, + feature="Feature", + generated_test_cases=cases, + ) + + assert "invalid TestRail submission" in str(exc.value) + assert recorder.actions == [] + + async def test_submit_test_plan_tool_preserves_blank_expectations(): recorder = ActionsRecorder() diff --git a/libs/hackbot-runtime/tests/test_testrail_handler.py b/libs/hackbot-runtime/tests/test_testrail_handler.py index 39100084d2..9fae0d8f56 100644 --- a/libs/hackbot-runtime/tests/test_testrail_handler.py +++ b/libs/hackbot-runtime/tests/test_testrail_handler.py @@ -17,7 +17,6 @@ def _plan(): { "id": 1, "title": "The PDF opens", - "context": "content", "preconditions": "A PDF is available.", "steps": [ {"action": "Open the PDF", "expectation": None}, @@ -30,7 +29,6 @@ def _plan(): { "id": 2, "title": "The toolbar remains available", - "context": "chrome", "preconditions": None, "steps": [ { diff --git a/services/hackbot-ui/components/FindingsView.tsx b/services/hackbot-ui/components/FindingsView.tsx index e3a981d1b2..bd3a3c2519 100644 --- a/services/hackbot-ui/components/FindingsView.tsx +++ b/services/hackbot-ui/components/FindingsView.tsx @@ -10,7 +10,6 @@ import { titleize, } from "@/lib/findings-format"; import { Markdown } from "./Markdown"; -import { parseTestPlan, TestPlanView } from "./TestPlanView"; type ViewMode = "friendly" | "raw"; @@ -211,14 +210,9 @@ function FriendlyFindings({ findings }: { findings: Record }) { export function FindingsView({ findings, - agent, }: { findings: Record; - agent: string; }) { - const testPlan = - agent === "test-plan-generator" ? parseTestPlan(findings) : null; - // Default to the friendly, readable view; raw JSON is opt-in. const [mode, setMode] = useState("friendly"); return ( @@ -247,11 +241,7 @@ export function FindingsView({ {mode === "friendly" ? ( - testPlan ? ( - - ) : ( - - ) + ) : ( 
{JSON.stringify(findings, null, 2)}
)} diff --git a/services/hackbot-ui/components/RunDetail.tsx b/services/hackbot-ui/components/RunDetail.tsx index b1351d1598..68b9a67faa 100644 --- a/services/hackbot-ui/components/RunDetail.tsx +++ b/services/hackbot-ui/components/RunDetail.tsx @@ -15,6 +15,7 @@ import { import { FindingsView } from "./FindingsView"; import { Markdown } from "./Markdown"; import { StatusBadge } from "./StatusBadge"; +import { parseTestPlan, TestPlanView } from "./TestPlanView"; // Proposed bugzilla.add_comment actions carry the comment body in params.text; // pull it out so we can preview what would be posted to the bug. @@ -162,6 +163,12 @@ export function RunDetail({ runId }: { runId: string }) { const log = extractLog(run); const findings = run.summary?.findings ?? {}; const hasFindings = Object.keys(findings).length > 0; + // The QA agent gets its own purpose-built view; its plan lives on the + // TestRail action, so findings are usually empty (raw data is in summary.json). + const testPlan = + run.agent === "test-plan-generator" + ? parseTestPlan(findings, actions) + : null; // Both pending and failed actions are (re)applied by the apply endpoint — it // skips only already-applied ones — so one button covers applying and retry. @@ -251,7 +258,11 @@ export function RunDetail({ runId }: { runId: string }) { )} - {hasFindings && } + {testPlan ? ( + + ) : ( + hasFindings && + )} {actions && actions.length > 0 && (
diff --git a/services/hackbot-ui/components/TestPlanView.tsx b/services/hackbot-ui/components/TestPlanView.tsx index bcaa49c7fe..044a622dec 100644 --- a/services/hackbot-ui/components/TestPlanView.tsx +++ b/services/hackbot-ui/components/TestPlanView.tsx @@ -1,4 +1,5 @@ import { isPlainObject } from "@/lib/findings-format"; +import type { RunAction } from "@/lib/types"; type TestCaseStatus = "passed" | "failed" | "unsuitable"; @@ -53,9 +54,18 @@ function isTestStep(value: TestStep | null): value is TestStep { } export function parseTestPlan( - findings: Record + findings: Record, + actions: RunAction[] | null = null ): TestPlan | null { - const result = findings.result; + // The agent now records the plan straight onto the action, so that is the + // source of truth. Runs from before the switch still carry it in findings. + const testRailAction = actions?.find( + (action) => + action.type === "testrail.submit_test_plan" && + isPlainObject(action.params) && + Array.isArray(action.params.generated_test_cases) + ); + const result = testRailAction?.params ?? findings.result; if (!isPlainObject(result) || !Array.isArray(result.generated_test_cases)) { return null; } @@ -118,7 +128,8 @@ export function TestPlanView({ testPlan }: { testPlan: TestPlan }) { ); return ( -
+
+

Test plan

{testPlan.feature && (

Feature name