From 69f6ec59f3a96d5da1ce47d371966699cc40f13c Mon Sep 17 00:00:00 2001 From: Cory Thomas Date: Fri, 21 Aug 2026 20:46:31 -0400 Subject: [PATCH 1/7] frontend-triage: send one area's source-tree guidance, not all eight The system prompt carried where-the-code-lives notes for every area on every run -- about a third of its size -- and grew with each new component. A New Tab Page bug carried 10,300 characters of it to use 128. Each area now has a file under rules/areas/, and the prompt gets the one the bug's component maps to. That takes a recognized component from 32,313 characters to roughly 20,000, and keeps it flat as components are added: a new one costs a file and an index line instead of taxing every other run. Three things keep this from being less reliable than the prompt it replaces: - An unknown component, or a failed lookup, gets every area. rules/scoping.md puts an unlisted component in scope, so guessing one area would leave those runs with less than they have today. The lookup goes through the broker, since the agent container holds no Bugzilla credentials, and fails open rather than failing. - Firefox :: Sharing gets Site permissions alongside it. A "stop sharing" report arrives there but is WebRTC, which site permissions owns. - A comment citing code from an area the agent never loaded is refused, naming the area and the tool that fetches it. load_area_guidance shares the loaded set with the hook so the retry succeeds. A path in no area passes -- a bug that turns out to be Graphics must not block on guidance that does not exist. The prose moved verbatim, checked paragraph by paragraph against the original. Two cross-references changed because they pointed at blocks no longer in the same file, and headings are demoted on injection so guidance nests under Source repository rather than reading as a new top-level section. --- .../hackbot_agents/frontend_triage/agent.py | 114 ++++++++++++++++- .../hackbot_agents/frontend_triage/areas.py | 73 +++++++++++ .../hackbot_agents/frontend_triage/config.py | 121 +++++++++++++++++- .../hackbot_agents/frontend_triage/hooks.py | 55 ++++++++ .../frontend_triage/prompts/system.md | 37 ++---- .../rules/areas/application-updater.md | 7 + .../rules/areas/desktop-frontend.md | 7 + .../rules/areas/firefox-for-android.md | 11 ++ .../rules/areas/ip-protection.md | 15 +++ .../rules/areas/messaging-system.md | 5 + .../frontend_triage/rules/areas/sharing.md | 9 ++ .../rules/areas/site-permissions.md | 7 + .../rules/areas/windows-installer.md | 7 + agents/frontend-triage/tests/test_hooks.py | 69 +++++++++- agents/frontend-triage/tests/test_plan.py | 84 ++++++++++-- 15 files changed, 574 insertions(+), 47 deletions(-) create mode 100644 agents/frontend-triage/hackbot_agents/frontend_triage/areas.py create mode 100644 agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/application-updater.md create mode 100644 agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/desktop-frontend.md create mode 100644 agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/firefox-for-android.md create mode 100644 agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/ip-protection.md create mode 100644 agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/messaging-system.md create mode 100644 agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/sharing.md create mode 100644 agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/site-permissions.md create mode 100644 agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/windows-installer.md diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py b/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py index cc2d93e35b..c3de0ff5e0 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py @@ -16,6 +16,7 @@ import json import re import sys +from collections.abc import Sequence from pathlib import Path from agent_tools import mozilla_vcs, searchfox @@ -41,21 +42,30 @@ permalink_prefix, resolve_index_revision, ) +from mcp import ClientSession +from mcp.client.streamable_http import streamablehttp_client from pydantic import BaseModel from searchfox import AsyncSearchfoxClient +from . import areas as area_tools +from .areas import AreaGuidanceContext from .config import ( + AREA_TOOLS, + AREAS, BUGZILLA_READ_TOOLS, ENABLED_ACTION_TYPES, MOZILLA_VCS_TOOLS, SEARCHFOX_TOOLS, TRIAGE_SCOPE, TRIAGE_SEVERITIES, + Area, ScopedComponent, + areas_for, ) -from .hooks import add_comment_hook, severity_block_hook +from .hooks import add_comment_hook, area_guidance_hook, severity_block_hook HERE = Path(__file__).resolve().parent +AREAS_DIR = HERE / "rules" / "areas" # The agent is asked to end its final message with a fenced ```json block # carrying the structured plan. We parse the last such block so the result is @@ -192,7 +202,78 @@ def render_scope(scope: tuple[ScopedComponent, ...] = TRIAGE_SCOPE) -> str: ) -def load_system_prompt(rules_dir: Path, extra: str) -> str: +async def fetch_product_component( + bugzilla_mcp_server: McpServerConfig, bug: int +) -> tuple[str | None, str | None]: + """The bug's product and component, read through the Bugzilla broker. + + Needed before the agent starts, because the system prompt is built once and the + area guidance goes into it -- see `areas_for`. The agent's own first step fetches + the bug too, but that is several turns after the prompt is frozen. + + Goes through the broker's MCP endpoint because that is the only Bugzilla path this + process has: the agent container binds no credentials (see `compose.yml`). + + Returns ``(None, None)`` on any failure, which `areas_for` turns into every area -- + today's prompt. Never raises: the guidance is an optimization, and a broken lookup + must not take down a run that would otherwise work. + """ + url = ( + bugzilla_mcp_server.get("url") + if isinstance(bugzilla_mcp_server, dict) + else None + ) + if not url: + return None, None + + try: + async with streamablehttp_client(url) as (read, write, _): + async with ClientSession(read, write) as session: + await session.initialize() + res = await session.call_tool( + "get_bugs", + {"ids": [bug], "include_fields": "product,component"}, + ) + payload = json.loads(res.content[0].text) + bugs = payload.get("bugs") or [] + if not bugs: + return None, None + return bugs[0].get("product"), bugs[0].get("component") + except Exception as e: # - see docstring; every failure fails open + print( + f"[frontend_triage] component lookup failed ({type(e).__name__}: {e}); " + f"sending every area's guidance", + file=sys.stderr, + ) + return None, None + + +def render_area_index() -> str: + """One line per area: its name and the trees it covers. + + Always in the prompt, even when only one area's guidance is. It is what lets the + agent recognise that the code it just localized into belongs to an area it does not + have, which is the trigger for `load_area_guidance`. + """ + return "\n".join(f"- **{a.name}** — {', '.join(a.trees)}" for a in AREAS) + + +def read_area_guidance(areas: Sequence[Area]) -> str: + """The `rules/areas/` files for ``areas``, concatenated for the prompt. + + Headings are demoted two levels on the way in. The files are `# ` because + `load_area_guidance` serves them whole, but pasted verbatim that H1 would sit + between `# Source repository` and `# Linking source files` and read as a new + top-level section rather than as part of the one it belongs to. + """ + bodies = [] + for area in areas: + text = (AREAS_DIR / f"{area.slug}.md").read_text().strip() + bodies.append(re.sub(r"^(#{1,4}) ", r"##\1 ", text, flags=re.MULTILINE)) + return "\n\n".join(bodies) + + +def load_system_prompt(rules_dir: Path, extra: str, areas: Sequence[Area]) -> str: tmpl = (HERE / "prompts" / "system.md").read_text() return tmpl.format( @@ -200,6 +281,8 @@ def load_system_prompt(rules_dir: Path, extra: str) -> str: extra_instructions=extra or "(none)", searchfox_links=SEARCHFOX_LINKS_PROMPT, triaged_components=render_scope(), + area_index=render_area_index(), + area_guidance=read_area_guidance(areas), ) @@ -510,13 +593,36 @@ async def run_frontend_triage( ) actions_recorder.add_hook("bugzilla.add_comment", severity_block_hook) + # Which areas' guidance goes in the prompt. Falls back to every area when the bug's + # component is unknown or the lookup failed, which is what the prompt carried before + # this was split up -- see `areas_for`. + product, component = await fetch_product_component(bugzilla_mcp_server, bug) + areas = areas_for(product, component) + loaded_areas = {area.name for area in areas} + print( + f"[frontend_triage] {product} :: {component} -> " + f"{', '.join(sorted(loaded_areas))}", + file=sys.stderr, + ) + + # Registered before `permalink_hook`, which rewrites the placeholders this reads. + actions_recorder.add_hook("bugzilla.add_comment", area_guidance_hook(loaded_areas)) + actions_recorder.add_hook( "bugzilla.add_comment", permalink_hook(permalink_prefix(searchfox_rev), source_repo.resolve()), ) actions_recorder.add_hook("bugzilla.add_comment", feedback_tags_hook) - system_prompt = load_system_prompt(rules_dir, instructions) + # Shares `loaded_areas` with the hook above, so an area the agent pulls mid-run + # stops the hook refusing a comment that cites it. + areas_server = build_sdk_server( + "areas", + AreaGuidanceContext(areas_dir=AREAS_DIR, loaded=loaded_areas), + area_tools.TOOLS, + ) + + system_prompt = load_system_prompt(rules_dir, instructions, areas) options = ClaudeAgentOptions( system_prompt=system_prompt, @@ -524,6 +630,7 @@ async def run_frontend_triage( "bugzilla": bugzilla_mcp_server, "searchfox": searchfox_server, "mozilla_vcs": vcs_server, + "areas": areas_server, ACTIONS_SERVER_NAME: actions_server, }, agents={ @@ -544,6 +651,7 @@ async def run_frontend_triage( *BUGZILLA_READ_TOOLS, *SEARCHFOX_TOOLS, *MOZILLA_VCS_TOOLS, + *AREA_TOOLS, *enabled_action_tools, ], model=model, diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/areas.py b/agents/frontend-triage/hackbot_agents/frontend_triage/areas.py new file mode 100644 index 0000000000..c886f3aa58 --- /dev/null +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/areas.py @@ -0,0 +1,73 @@ +"""The ``load_area_guidance`` tool -- read a `rules/areas/` file mid-run. + +The prompt carries the guidance for the area the bug's component maps to. This is how +the agent gets a *different* one when its investigation says the code lives elsewhere, +and it records what was loaded so `hooks.area_guidance_hook` can tell whether a comment +is citing a tree the agent was told nothing about. + +A plain ``Read`` of the file would work equally well for the agent and not at all for +the hook, which needs the load to be observable. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path +from typing import Annotated + +from agent_tools.registry import ToolError, tool, tools_in +from pydantic import Field + +from .config import AREAS, AREAS_BY_NAME + + +@dataclass +class AreaGuidanceContext: + """Where the area files live, and which ones this run has loaded. + + ``loaded`` is shared with the hook rather than copied: it starts as the areas that + were injected into the prompt, and each successful call adds to it. + """ + + areas_dir: Path + loaded: set[str] + + +@tool +async def load_area_guidance( + ctx: AreaGuidanceContext, + area: Annotated[ + str, + Field( + description=( + "Area name, exactly as listed in the system prompt's area index -- " + "e.g. 'Windows installer', 'Site permissions'." + ) + ), + ], +) -> dict: + """Read the source-tree guidance for one area. + + Call this when your investigation shows the bug's code is in an area whose guidance + is not already in your prompt. Recording a comment that cites files from an + unloaded area is refused. + """ + entry = AREAS_BY_NAME.get(area) or next( + (a for a in AREAS if a.name.casefold() == area.casefold()), None + ) + if entry is None: + raise ToolError( + f"no area named {area!r}", + payload={ + "error": "unknown_area", + "requested": area, + "known_areas": [a.name for a in AREAS], + }, + ) + + text = (ctx.areas_dir / f"{entry.slug}.md").read_text() + ctx.loaded.add(entry.name) + return {"area": entry.name, "guidance": text} + + +TOOLS = tools_in(__name__) diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py index c70d1dce36..072b4c5705 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py @@ -31,6 +31,14 @@ ] +# Per-area source-tree guidance (in-process MCP server "areas"). Only reached when the +# agent localizes outside the area its component maps to; the usual case is already in +# the prompt. +AREA_TOOLS = [ + "mcp__areas__load_area_guidance", +] + + # Recordable action types the agent may take, by dotted id. A comment is the only one: # `bugzilla.update_bug` was here for `severity`, which is now a suggestion in the comment # instead, leaving the tool with no caller. @@ -48,20 +56,83 @@ class ScopedComponent(NamedTuple): product: str component: str - # Which `Source repository` bullet in prompts/system.md describes this component's - # code. `tests/test_plan.py` asserts every area named here has one, so a new area - # cannot be added without the guidance that makes it triageable. + # Which `rules/areas/` file describes this component's code. `tests/test_plan.py` + # asserts every area named here has one, so a new area cannot be added without the + # guidance that makes it triageable. area: str # Required, because an entry without one would be a component getting unattended # triage with nobody told -- which is what `channel_for` failing closed produces, # and not something to be able to express by accident. channel: str + # Areas whose guidance goes in the prompt alongside `area`, for components that + # routinely turn out to be somewhere else. Sharing is the known one: a "stop + # sharing" report arrives here but is WebRTC, which site permissions owns. Listing + # the pair means both files are present from the start, rather than the agent + # having to notice mid-run and fetch the second one. + related_areas: tuple[str, ...] = () @property def key(self) -> str: return f"{self.product} :: {self.component}" +class Area(NamedTuple): + """One `rules/areas/` guidance file, and the trees whose code it describes.""" + + name: str + slug: str + # Path prefixes this area owns. Longest match wins in `area_for_path`, so a nested + # area resolves ahead of the tree containing it -- `browser/installer/` is the + # Windows installer, not the desktop frontend's `browser/`. + trees: tuple[str, ...] + + +# Every area, in the order they are listed to the model. `slug` is the filename under +# `rules/areas/`; `trees` drives both that index and `hooks.area_guidance_hook`. +AREAS = ( + Area("Desktop frontend", "desktop-frontend", ("browser/", "toolkit/", "devtools/")), + Area( + "Site permissions", + "site-permissions", + ( + "browser/modules/SitePermissions.sys.mjs", + "browser/modules/PermissionUI.sys.mjs", + "browser/actors/WebRTCParent.sys.mjs", + "browser/components/preferences/dialogs/", + "extensions/permissions/", + ), + ), + Area( + "Sharing", + "sharing", + ( + "browser/modules/SharingUtils.sys.mjs", + "browser/components/contentsharing/", + "widget/", + ), + ), + Area( + "IP Protection", + "ip-protection", + ("browser/components/ipprotection/", "toolkit/components/ipprotection/"), + ), + Area( + "Messaging System", + "messaging-system", + ( + "browser/components/asrouter/", + "browser/components/aboutwelcome/", + "toolkit/components/messaging-system/", + ), + ), + Area("Firefox for Android", "firefox-for-android", ("mobile/android/",)), + Area("Application updater", "application-updater", ("toolkit/mozapps/update/",)), + Area("Windows installer", "windows-installer", ("browser/installer/",)), +) + +AREAS_BY_NAME = {a.name: a for a in AREAS} + + # The components that are sent here for triage, and the channel that owns each. The # single source of truth for both: `SLACK_CHANNELS` below is derived from it, and # `render_scope` in agent.py renders it into the system prompt, so adding a component is @@ -89,7 +160,13 @@ def key(self) -> str: ScopedComponent( "Firefox", "Site Permissions", "Site permissions", "#privacy-team-automation" ), - ScopedComponent("Firefox", "Sharing", "Sharing", "#content-sharing-automation"), + ScopedComponent( + "Firefox", + "Sharing", + "Sharing", + "#content-sharing-automation", + related_areas=("Site permissions",), + ), ScopedComponent( "Firefox", "IP Protection", @@ -124,6 +201,42 @@ def key(self) -> str: # that `notify.py` keeps one flat mapping to look up. SLACK_CHANNELS = {c.key: c.channel for c in TRIAGE_SCOPE} +_SCOPE_BY_KEY = {c.key: c for c in TRIAGE_SCOPE} + + +def areas_for(product: str | None, component: str | None) -> tuple[Area, ...]: + """The areas whose guidance belongs in the prompt for a bug in this component. + + **Every** area when the component is not one we triage, or when the caller could + not determine it. `rules/scoping.md` is explicit that a defect in an unlisted + component is still in scope, and a run that guessed one area for such a bug would + have less to work with than it does today. Failing open costs the current prompt + size and nothing else, so it is the only safe default. + """ + entry = _SCOPE_BY_KEY.get( + f"{(product or '').strip()} :: {(component or '').strip()}" + ) + if entry is None: + return AREAS + return tuple(AREAS_BY_NAME[name] for name in (entry.area, *entry.related_areas)) + + +def area_for_path(path: str) -> Area | None: + """The area owning ``path``, or None if no area does. + + None is the ordinary answer for a file outside the triaged areas -- `gfx/`, say -- + and it means "no guidance exists for this", not "guidance is missing". Longest + prefix wins so `browser/installer/...` resolves to the installer rather than to the + desktop frontend's `browser/`. + """ + best: tuple[int, Area] | None = None + for area in AREAS: + for tree in area.trees: + if path.startswith(tree) and (best is None or len(tree) > best[0]): + best = (len(tree), area) + return best[1] if best else None + + # Bugzilla's `bug_severity` legal values are `--`, `blocker`, `S1`, `critical`, # `S2`, `major`, `normal`, `S3`, `minor`, `S4`, `trivial`, `N/A`, `enhancement` # (https://bugzilla.mozilla.org/rest/field/bug/bug_severity). Narrowed to the four diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/hooks.py b/agents/frontend-triage/hackbot_agents/frontend_triage/hooks.py index 50fbcc86b8..848baff363 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/hooks.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/hooks.py @@ -24,6 +24,8 @@ from agent_tools.registry import ToolError from hackbot_runtime.actions import ActionHook, ActionsRecorder +from .config import area_for_path + # A line the model writes to declare its severity. Anchored to the line start so an # ordinary mention -- quoting a reporter, or arguing why something is not S1 -- does # not count as a second declaration. @@ -46,6 +48,59 @@ def severity_block_hook(action: dict) -> None: ) +# Source paths reach the comment as the Searchfox placeholder plus a repo-relative +# path (see `SEARCHFOX_LINKS_PROMPT`), which is why this hook has to run before +# `permalink_hook` rewrites them into URLs. +_CITED_PATH = re.compile(r"\{\{\s*searchfox\.permalink\s*\}\}/?(?P[^)\s#]+)") + + +def cited_paths(text: str) -> list[str]: + """Every repo-relative source path the comment links to.""" + return [m.group("path") for m in _CITED_PATH.finditer(text)] + + +def area_guidance_hook(loaded_areas: set[str]) -> ActionHook: + """Refuse a comment citing code from an area whose guidance was never loaded. + + The prompt carries one area's guidance, chosen from the bug's component before the + run starts. When the investigation lands somewhere else -- a New Tab Page bug that + turns out to be the installer -- the agent has to call `load_area_guidance` for it, + and nothing but this hook makes that reliable. Without it the run would quietly + fix-plan a tree it was told nothing about, which is worse than what it does today + with every area in the prompt. + + ``loaded_areas`` is shared with the ``areas`` MCP server, which adds to it as the + agent loads files; it starts as whatever was injected. + + A path in **no** area passes. `gfx/` has no guidance to load, and that case works + today -- the agent localizes from the tree and Searchfox. Blocking on it would fail + a run over something the agent cannot possibly satisfy. + """ + + def hook(action: dict) -> None: + text = (action.get("params") or {}).get("text") + if not isinstance(text, str): + return + + missing: dict[str, str] = {} + for path in cited_paths(text): + area = area_for_path(path) + if area is not None and area.name not in loaded_areas: + missing.setdefault(area.name, path) + + if missing: + named = ", ".join( + f"{area} (e.g. {path})" for area, path in sorted(missing.items()) + ) + raise ToolError( + f"your comment cites code in {named}, whose guidance you have not " + f"read. Call `load_area_guidance` for each, then revise the comment " + f"against what it says -- your localization may be wrong." + ) + + return hook + + def _check_no_comment_yet(recorder: ActionsRecorder) -> None: # The rules ask for a single comment, but nothing else caps the count, and the # agent reads every comment on the bug as untrusted input. A run told to write diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/prompts/system.md b/agents/frontend-triage/hackbot_agents/frontend_triage/prompts/system.md index 6047512b31..7824ce0029 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/prompts/system.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/prompts/system.md @@ -36,34 +36,15 @@ Use **only** these tools for accessing Bugzilla, nothing else. Your working directory is the Firefox source repository — the whole tree, desktop and Android in one checkout. You have Read, Grep, Glob, and Bash (read-only — do not modify files) to inspect it. Use this to localize the bug: find the modules, markup, styling, and prefs (often under `modules/libpref/init/all.js`) that govern the behaviour, and any existing tests that cover the area. -Where to look, and what you will find there, depends on the bug's component: - -- **Desktop frontend** — `browser/`, `toolkit/`, and `devtools/`. JS/JSM modules (`.js`, `.mjs`, `.sys.mjs`), CSS, and XUL/HTML. -- **Site permissions** — desktop JS, but split across the prompt, the state, and the store, so start by working out which of the three the bug is in. `browser/modules/SitePermissions.sys.mjs` holds the permission state the rest of the frontend reads and writes, including the defaults, the scopes (`SCOPE_PERSISTENT`, `SCOPE_SESSION`, `SCOPE_TEMPORARY`), and the `ALLOW`/`BLOCK`/`PROMPT` states. `browser/modules/PermissionUI.sys.mjs` builds the doorhanger prompts, one subclass per permission type. `browser/actors/WebRTCParent.sys.mjs` handles camera, microphone, and screen sharing, which do **not** go through the generic prompt path and carry their own sharing indicator. The management UI is `browser/components/preferences/dialogs/permissions.js` and `sitePermissions.js`. The backing store is `nsIPermissionManager`, implemented in C++ at `extensions/permissions/PermissionManager.cpp` — that is outside the frontend directories, so "the permission did not stick", "it came back after a restart", and wrong-expiry bugs are localized there and are **not** out of scope for being non-JS. -- **Sharing** — sending the current page to another app, and the one area here whose code reaches outside `browser/`, `toolkit/` and `devtools/`. `browser/modules/SharingUtils.sys.mjs` is the frontend: it populates the share menu, gates on `BrowserUtils.getShareableURL` (which is why an unshareable scheme silently yields no menu item), and then hands off to the platform. `browser/components/contentsharing/` is the newer piece — `ContentSharingUtils.sys.mjs`, the remotely-delivered config validated against `contentsharing.schema.json`, `content/`, and its own `metrics.yaml`. **The platform half is in `widget/`**, which the desktop-frontend bullet above does not cover: `widget/nsIMacSharingService.idl` with `widget/cocoa/nsMacSharingService.mm` (Objective-C++ — the macOS share sheet, `getSharingProviders`, `openSharingPreferences`), and `widget/nsIWindowsUIUtils.idl`'s `shareUrl` for Windows. So "the Share menu is empty", "the wrong apps are listed", and "Share does nothing" are usually localized in `widget/`, per-OS, and are **not** out of scope for being C++ rather than JS. Note which OS the bug is about before reading either. - - **Two unrelated things are called "sharing" in this tree.** This component is sharing a URL _out_ to another app. Screen, camera and microphone sharing — the sharing indicator, "stop sharing" button, and per-tab sharing state — is WebRTC, lives in `browser/actors/WebRTCParent.sys.mjs`, and belongs to site permissions. A grep for `sharing` returns both, so check which one the report is actually about; a bug about an indicator or a "stop sharing" control is almost certainly the WebRTC one. -- **IP Protection** — the built-in VPN, desktop JS in two trees, and which tree matters more than which file. `browser/components/ipprotection/` is the UI and the per-window glue: `IPProtection.sys.mjs` (`EveryWindow` and `CustomizableUI` registration), `IPProtectionPanel.sys.mjs` (panel lifecycle and the only sanctioned way to change what the panel shows, `setState`), `IPProtectionToolbarButton.sys.mjs`, `IPProtectionInfobarManager.sys.mjs`, `IPProtectionAlertManager.sys.mjs`, and one-concern `IPP*Helper.sys.mjs` files for onboarding, opt-out, and usage. The panel's own markup is Lit components under `content/*.mjs` (`ipprotection-content.mjs`, `ipprotection-status-card.mjs`, `ipprotection-locations.mjs`, `ipprotection-message-bar.mjs`), with shared values — thresholds, URLs, country-to-flag maps — in `content/ipprotection-constants.mjs`. `toolkit/components/ipprotection/` is the platform-agnostic service layer: `IPProtectionService.sys.mjs`, `IPPProxyManager.sys.mjs`, `IPPChannelFilter.sys.mjs` (which traffic is proxied), `IPPNetworkErrorObserver.sys.mjs`, `IPProtectionServerlist.sys.mjs`, `IPPAuthProvider.sys.mjs`, `IPPExceptionsManager.sys.mjs` (per-site exclusions), `IPPNimbusHelper.sys.mjs`. - - **State lives in the service, not the panel**, so a bug whose symptom is in the panel usually is not. There are **two** state machines and both have a `READY`: `IPProtectionStates` in `IPProtectionService.sys.mjs` is entitlement and sign-in (`UNINITIALIZED`, `UNAVAILABLE`, `UNAUTHENTICATED`, `READY`) and fires `IPProtectionService:StateChanged`; `IPPProxyStates` in `IPPProxyManager.sys.mjs` is the connection (`NOT_READY`, `READY`, `ACTIVATING`, `ACTIVE`, `ERROR`, `PAUSED`) and fires `IPPProxyManager:StateChanged`. Say which one you mean. "It showed connected when it was not" and "it came back on after I turned it off" are proxy-state bugs in `toolkit/`; "the panel offered it to a user who is not entitled" is a service-state bug. The panel only reacts, through `setState`, and content components emit `IPProtection:*` events upward rather than acting. - - `toolkit/components/ipprotection/docs/` has `StateMachine.rst`, `Preferences.rst`, `Constants.rst` and `Components.rst` — in-tree prose documentation, which none of the other areas here has. **Read it before reasoning about a state transition**; it is faster and more reliable than reconstructing the machine from the source. - - A `browser/` → `toolkit/` split is in progress, so both trees can hold a plausible-looking copy of the same concern and the shallow local checkout may be behind. Prefer `search_identifier` / `find_definition`, which see the indexed revision, before citing a path. - - Prefs are `browser.ipProtection.*`, registered in `browser/app/profile/firefox.js` — **not** `modules/libpref/init/all.js`. Strings are `browser/locales/en-US/browser/ipProtection.ftl`, and Glean metrics are in a `metrics.yaml` in each of the two directories. -- **Messaging System** — the in-product messaging surfaces: about:welcome, feature callouts, Spotlight modal dialogs, and Infobars. Desktop JS/JSM, CSS and XUL/HTML across three trees: `browser/components/asrouter/` (the router that decides which message shows, and the templates it shows them in), `browser/components/aboutwelcome/` (the onboarding and first-run flow), and `toolkit/components/messaging-system/` (the platform-agnostic layer, including the **JSON Schemas** message definitions are validated against). The router is shared by every surface, so work out which surface the reporter was on before reading any of the three — a bug in one surface is usually not in the router. - - **A message is data, not code.** Message definitions are delivered remotely and matched to a user by targeting expressions, so "I saw the wrong message", "I saw it twice" and "I never saw it" are usually a message-definition, targeting or frequency-cap problem rather than a defect in this tree. Say which of the two you think it is, and if it is the message rather than the code, say what would confirm that instead of planning a change against a definition you cannot see. A rendering or interaction bug in the surface itself is the ordinary case and localizes normally. -- **Firefox for Android** — `mobile/android/`, with the Fenix app under `mobile/android/fenix/app/src/main/java/org/mozilla/fenix/` and the reusable components under `mobile/android/android-components/`. This is **Kotlin**, and it is structured as Fragment / Store / Middleware / View rather than as chrome markup plus a script: a `…Fragment.kt` owns the screen, a `…FragmentStore.kt` holds its state and actions, a `…View.kt` or a Compose function renders it, and a `…Middleware.kt` performs side effects. Layouts are Android XML under `mobile/android/fenix/app/src/main/res/layout/`, strings under `res/values/strings.xml`. Fenix is mid-migration to Jetpack Compose, so a screen may have both a `…View.kt` and a `…Composable.kt` and only one of them is live — check which the Fragment actually builds before planning against either. - - **Android toolbar** — there are **two** toolbars, and a generation of the widget under each. The browser toolbar is `…/fenix/components/toolbar/` (`BrowserToolbarComposable.kt`, `BrowserToolbarMiddleware.kt`, `BrowserNavigationBar.kt`, `ToolbarPosition.kt` for top-versus-bottom, `BottomToolbarContainerView.kt`, `ToolbarsIntegration.kt`); the homepage has its own at `…/fenix/home/toolbar/` (`HomeToolbarComposable.kt`, `FenixHomeToolbar.kt`, `BrowserSimpleToolbar.kt`). So work out which surface the reporter was on first: a `Homepage` bug can localize into a toolbar file and a `Toolbar` bug into the homepage. Underneath both, android-components has the newer Compose widget at `mobile/android/android-components/components/compose/browser-toolbar/` and the older View-based one at `components/browser/toolbar/`, with `components/concept/toolbar/` holding the interface and `components/feature/toolbar/` the session wiring. Confirm which one Fenix builds before citing it — a fix planned against the retired implementation reads correct and changes nothing. - - **Android homepage** — one screen assembled from one package per section, so "which section" comes before "which file". `…/fenix/home/HomeFragment.kt` owns the screen, the Compose UI is under `home/ui/` (`Homepage.kt`, `HomepageHeader.kt`, `SearchBar.kt`, `WallpaperBackground.kt`, `Wordmark.kt`), state is `home/store/HomepageState.kt`, side effects are `home/middleware/`, and the older controller/interactor pair is `home/sessioncontrol/`. Each section is its own subpackage: `topsites/`, `recenttabs/`, `recentsyncedtabs/`, `recentvisits/`, `pocket/`, `bookmarks/`, `collections/`, `setup/`, `sports/`, `mars/`, `logo/`, `privatebrowsing/`. A bug about the top-sites row or the stories feed is localized there, not in `Homepage.kt`. Note also that `Firefox for Android` has separate components for several of these sections — `Top Sites`, `Stories`, `Collections`, `Bookmarks`, `Menu`, `Search` — so the same code can be reached from more than one component, and `Stories` is `home/pocket/` in the tree because nothing was renamed. Triage the bug under the component it was filed in; do not retitle or re-scope it to match. -- **Application updater** — `toolkit/mozapps/update/`. `.sys.mjs` modules (`AppUpdater.sys.mjs`, `UpdateService.sys.mjs`, `BackgroundUpdate.sys.mjs`), the XPCOM interfaces in `nsIUpdateService.idl`, and the C++ updater binary under `toolkit/mozapps/update/updater/`. Update behaviour is heavily driven by prefs under `app.update.*` and by the state written to the update directory, so read `common/` for the shared constants and status codes. -- **Windows installer** — `browser/installer/windows/nsis/`. This is **NSIS**: `installer.nsi` (the full installer), `stub.nsi` (the small downloader stub), `uninstaller.nsi`, `maintenanceservice_installer.nsi`, and the `.nsh` include files that hold most of the logic. Localized strings live in the `.nsi`/`.properties` files alongside. The packaging manifests are `browser/installer/package-manifest.in` and `browser/installer/allowed-dupes.mn`, and the MSI and MSIX wrappers are in the sibling `msi/` and `msix/` directories. There is no JS here at all. Note which installer the bug is about: the stub and the full installer are separate programs with separate code. - -**Always look for an existing test that exercises the affected area**, and record what you find in the `relevant_tests` field — it is the downstream executor's verification anchor. Where to look depends on the component: - -- Desktop: browser-chrome mochitests usually live in a component's `tests/browser/` directory; also check `tests/`/`test/` and xpcshell tests. -- Sharing: browser-chrome under `browser/components/contentsharing/tests/browser/`, which has a `ContentSharingMockServer.sys.mjs` for the remote config — use it rather than stubbing the fetch yourself. Schema fixtures are xpcshell under `tests/unit/` (`validContentSharing.*.json` / `invalidContentSharing.*.json`), so a config-parsing bug has a very cheap regression test. The `widget/` half is effectively uncovered: there is no automated test for the macOS share sheet or the Windows share dialog, so for a platform-side bug say the area is untested rather than leaving the reader wondering. -- IP Protection: browser-chrome under `browser/components/ipprotection/tests/browser/`, which is where most of the coverage is, with shared setup in its `head.js` (`openPanel`, `closePanel`, and the panel-state helpers) — a new test almost always belongs there rather than in a bespoke setup. Also `browser/components/ipprotection/tests/xpcshell/` and, for the service layer, `toolkit/components/ipprotection/tests/xpcshell/`. Name the one matching the layer you localized to. -- Site permissions: the prompts are covered by browser-chrome under `browser/base/content/test/permissions/`, `SitePermissions.sys.mjs` itself by `browser/modules/test/browser/`, and the store by xpcshell under `extensions/permissions/test/`. Name the one that matches the layer you localized to, not whichever you found first. -- Android: Kotlin unit tests under `mobile/android/fenix/app/src/test/java/org/mozilla/fenix/`, and instrumented UI tests under `app/src/androidTest/`. The test tree mirrors the source packages, so name the mirror of the package you localized to — `…/test/java/org/mozilla/fenix/components/toolbar/` for the browser toolbar, `…/fenix/home/topsites/` for a top-sites bug — rather than the screen-level `HomeFragmentTest.kt`. A Compose surface may be covered only by an `androidTest` UI test; say so rather than reporting no coverage. -- Updater: `toolkit/mozapps/update/tests/` — xpcshell under `unit_aus_update/`, `unit_background_update/`, and `unit_update_binary/`, browser-chrome under `browser/`, plus `marionette/` and C++ `gtest/`. -- Installer: coverage is thin and specific. `browser/installer/windows/nsis/test/xpcshell/test_stub_installer.js` drives `test_stub.nsi` and covers the **stub** installer only; nothing exercises `installer.nsi` or the uninstaller. So for most Installer bugs an empty `relevant_tests` is the correct answer — say that the area is uncovered rather than leaving the reader to wonder whether you looked. +Where to look, and what you will find there, depends on the bug's component. Every area and the trees it covers: + +{area_index} + +Guidance for this bug's area follows. If your investigation shows the code is in a different area, call `load_area_guidance` with that area's name before you write the fix plan — a comment citing files from an area you have not loaded will be refused. A file in no area at all (`gfx/`, say) needs no load; there is nothing to fetch. + +{area_guidance} + +**Always look for an existing test that exercises the affected area**, and record what you find in the `relevant_tests` field — it is the downstream executor's verification anchor. Where to look is in the area guidance above. If you searched and there is genuinely no covering test, say so (empty `relevant_tests`). diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/application-updater.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/application-updater.md new file mode 100644 index 0000000000..34187f50db --- /dev/null +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/application-updater.md @@ -0,0 +1,7 @@ +# Application updater + +`toolkit/mozapps/update/`. `.sys.mjs` modules (`AppUpdater.sys.mjs`, `UpdateService.sys.mjs`, `BackgroundUpdate.sys.mjs`), the XPCOM interfaces in `nsIUpdateService.idl`, and the C++ updater binary under `toolkit/mozapps/update/updater/`. Update behaviour is heavily driven by prefs under `app.update.*` and by the state written to the update directory, so read `common/` for the shared constants and status codes. + +## Tests + +`toolkit/mozapps/update/tests/` — xpcshell under `unit_aus_update/`, `unit_background_update/`, and `unit_update_binary/`, browser-chrome under `browser/`, plus `marionette/` and C++ `gtest/`. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/desktop-frontend.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/desktop-frontend.md new file mode 100644 index 0000000000..f4cd46738d --- /dev/null +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/desktop-frontend.md @@ -0,0 +1,7 @@ +# Desktop frontend + +`browser/`, `toolkit/`, and `devtools/`. JS/JSM modules (`.js`, `.mjs`, `.sys.mjs`), CSS, and XUL/HTML. + +## Tests + +browser-chrome mochitests usually live in a component's `tests/browser/` directory; also check `tests/`/`test/` and xpcshell tests. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/firefox-for-android.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/firefox-for-android.md new file mode 100644 index 0000000000..8542d9195f --- /dev/null +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/firefox-for-android.md @@ -0,0 +1,11 @@ +# Firefox for Android + +`mobile/android/`, with the Fenix app under `mobile/android/fenix/app/src/main/java/org/mozilla/fenix/` and the reusable components under `mobile/android/android-components/`. This is **Kotlin**, and it is structured as Fragment / Store / Middleware / View rather than as chrome markup plus a script: a `…Fragment.kt` owns the screen, a `…FragmentStore.kt` holds its state and actions, a `…View.kt` or a Compose function renders it, and a `…Middleware.kt` performs side effects. Layouts are Android XML under `mobile/android/fenix/app/src/main/res/layout/`, strings under `res/values/strings.xml`. Fenix is mid-migration to Jetpack Compose, so a screen may have both a `…View.kt` and a `…Composable.kt` and only one of them is live — check which the Fragment actually builds before planning against either. + +**Android toolbar** — there are **two** toolbars, and a generation of the widget under each. The browser toolbar is `…/fenix/components/toolbar/` (`BrowserToolbarComposable.kt`, `BrowserToolbarMiddleware.kt`, `BrowserNavigationBar.kt`, `ToolbarPosition.kt` for top-versus-bottom, `BottomToolbarContainerView.kt`, `ToolbarsIntegration.kt`); the homepage has its own at `…/fenix/home/toolbar/` (`HomeToolbarComposable.kt`, `FenixHomeToolbar.kt`, `BrowserSimpleToolbar.kt`). So work out which surface the reporter was on first: a `Homepage` bug can localize into a toolbar file and a `Toolbar` bug into the homepage. Underneath both, android-components has the newer Compose widget at `mobile/android/android-components/components/compose/browser-toolbar/` and the older View-based one at `components/browser/toolbar/`, with `components/concept/toolbar/` holding the interface and `components/feature/toolbar/` the session wiring. Confirm which one Fenix builds before citing it — a fix planned against the retired implementation reads correct and changes nothing. + +**Android homepage** — one screen assembled from one package per section, so "which section" comes before "which file". `…/fenix/home/HomeFragment.kt` owns the screen, the Compose UI is under `home/ui/` (`Homepage.kt`, `HomepageHeader.kt`, `SearchBar.kt`, `WallpaperBackground.kt`, `Wordmark.kt`), state is `home/store/HomepageState.kt`, side effects are `home/middleware/`, and the older controller/interactor pair is `home/sessioncontrol/`. Each section is its own subpackage: `topsites/`, `recenttabs/`, `recentsyncedtabs/`, `recentvisits/`, `pocket/`, `bookmarks/`, `collections/`, `setup/`, `sports/`, `mars/`, `logo/`, `privatebrowsing/`. A bug about the top-sites row or the stories feed is localized there, not in `Homepage.kt`. Note also that `Firefox for Android` has separate components for several of these sections — `Top Sites`, `Stories`, `Collections`, `Bookmarks`, `Menu`, `Search` — so the same code can be reached from more than one component, and `Stories` is `home/pocket/` in the tree because nothing was renamed. Triage the bug under the component it was filed in; do not retitle or re-scope it to match. + +## Tests + +Kotlin unit tests under `mobile/android/fenix/app/src/test/java/org/mozilla/fenix/`, and instrumented UI tests under `app/src/androidTest/`. The test tree mirrors the source packages, so name the mirror of the package you localized to — `…/test/java/org/mozilla/fenix/components/toolbar/` for the browser toolbar, `…/fenix/home/topsites/` for a top-sites bug — rather than the screen-level `HomeFragmentTest.kt`. A Compose surface may be covered only by an `androidTest` UI test; say so rather than reporting no coverage. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/ip-protection.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/ip-protection.md new file mode 100644 index 0000000000..5c5ef8659f --- /dev/null +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/ip-protection.md @@ -0,0 +1,15 @@ +# IP Protection + +the built-in VPN, desktop JS in two trees, and which tree matters more than which file. `browser/components/ipprotection/` is the UI and the per-window glue: `IPProtection.sys.mjs` (`EveryWindow` and `CustomizableUI` registration), `IPProtectionPanel.sys.mjs` (panel lifecycle and the only sanctioned way to change what the panel shows, `setState`), `IPProtectionToolbarButton.sys.mjs`, `IPProtectionInfobarManager.sys.mjs`, `IPProtectionAlertManager.sys.mjs`, and one-concern `IPP*Helper.sys.mjs` files for onboarding, opt-out, and usage. The panel's own markup is Lit components under `content/*.mjs` (`ipprotection-content.mjs`, `ipprotection-status-card.mjs`, `ipprotection-locations.mjs`, `ipprotection-message-bar.mjs`), with shared values — thresholds, URLs, country-to-flag maps — in `content/ipprotection-constants.mjs`. `toolkit/components/ipprotection/` is the platform-agnostic service layer: `IPProtectionService.sys.mjs`, `IPPProxyManager.sys.mjs`, `IPPChannelFilter.sys.mjs` (which traffic is proxied), `IPPNetworkErrorObserver.sys.mjs`, `IPProtectionServerlist.sys.mjs`, `IPPAuthProvider.sys.mjs`, `IPPExceptionsManager.sys.mjs` (per-site exclusions), `IPPNimbusHelper.sys.mjs`. + +**State lives in the service, not the panel**, so a bug whose symptom is in the panel usually is not. There are **two** state machines and both have a `READY`: `IPProtectionStates` in `IPProtectionService.sys.mjs` is entitlement and sign-in (`UNINITIALIZED`, `UNAVAILABLE`, `UNAUTHENTICATED`, `READY`) and fires `IPProtectionService:StateChanged`; `IPPProxyStates` in `IPPProxyManager.sys.mjs` is the connection (`NOT_READY`, `READY`, `ACTIVATING`, `ACTIVE`, `ERROR`, `PAUSED`) and fires `IPPProxyManager:StateChanged`. Say which one you mean. "It showed connected when it was not" and "it came back on after I turned it off" are proxy-state bugs in `toolkit/`; "the panel offered it to a user who is not entitled" is a service-state bug. The panel only reacts, through `setState`, and content components emit `IPProtection:*` events upward rather than acting. + +`toolkit/components/ipprotection/docs/` has `StateMachine.rst`, `Preferences.rst`, `Constants.rst` and `Components.rst` — in-tree prose documentation, which none of the other triage areas has. **Read it before reasoning about a state transition**; it is faster and more reliable than reconstructing the machine from the source. + +A `browser/` → `toolkit/` split is in progress, so both trees can hold a plausible-looking copy of the same concern and the shallow local checkout may be behind. Prefer `search_identifier` / `find_definition`, which see the indexed revision, before citing a path. + +Prefs are `browser.ipProtection.*`, registered in `browser/app/profile/firefox.js` — **not** `modules/libpref/init/all.js`. Strings are `browser/locales/en-US/browser/ipProtection.ftl`, and Glean metrics are in a `metrics.yaml` in each of the two directories. + +## Tests + +browser-chrome under `browser/components/ipprotection/tests/browser/`, which is where most of the coverage is, with shared setup in its `head.js` (`openPanel`, `closePanel`, and the panel-state helpers) — a new test almost always belongs there rather than in a bespoke setup. Also `browser/components/ipprotection/tests/xpcshell/` and, for the service layer, `toolkit/components/ipprotection/tests/xpcshell/`. Name the one matching the layer you localized to. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/messaging-system.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/messaging-system.md new file mode 100644 index 0000000000..a327283bd3 --- /dev/null +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/messaging-system.md @@ -0,0 +1,5 @@ +# Messaging System + +the in-product messaging surfaces: about:welcome, feature callouts, Spotlight modal dialogs, and Infobars. Desktop JS/JSM, CSS and XUL/HTML across three trees: `browser/components/asrouter/` (the router that decides which message shows, and the templates it shows them in), `browser/components/aboutwelcome/` (the onboarding and first-run flow), and `toolkit/components/messaging-system/` (the platform-agnostic layer, including the **JSON Schemas** message definitions are validated against). The router is shared by every surface, so work out which surface the reporter was on before reading any of the three — a bug in one surface is usually not in the router. + +**A message is data, not code.** Message definitions are delivered remotely and matched to a user by targeting expressions, so "I saw the wrong message", "I saw it twice" and "I never saw it" are usually a message-definition, targeting or frequency-cap problem rather than a defect in this tree. Say which of the two you think it is, and if it is the message rather than the code, say what would confirm that instead of planning a change against a definition you cannot see. A rendering or interaction bug in the surface itself is the ordinary case and localizes normally. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/sharing.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/sharing.md new file mode 100644 index 0000000000..c650b8e214 --- /dev/null +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/sharing.md @@ -0,0 +1,9 @@ +# Sharing + +sending the current page to another app, and the one area here whose code reaches outside `browser/`, `toolkit/` and `devtools/`. `browser/modules/SharingUtils.sys.mjs` is the frontend: it populates the share menu, gates on `BrowserUtils.getShareableURL` (which is why an unshareable scheme silently yields no menu item), and then hands off to the platform. `browser/components/contentsharing/` is the newer piece — `ContentSharingUtils.sys.mjs`, the remotely-delivered config validated against `contentsharing.schema.json`, `content/`, and its own `metrics.yaml`. **The platform half is in `widget/`**, which the desktop frontend trees (`browser/`, `toolkit/`, `devtools/`) do not cover: `widget/nsIMacSharingService.idl` with `widget/cocoa/nsMacSharingService.mm` (Objective-C++ — the macOS share sheet, `getSharingProviders`, `openSharingPreferences`), and `widget/nsIWindowsUIUtils.idl`'s `shareUrl` for Windows. So "the Share menu is empty", "the wrong apps are listed", and "Share does nothing" are usually localized in `widget/`, per-OS, and are **not** out of scope for being C++ rather than JS. Note which OS the bug is about before reading either. + +**Two unrelated things are called "sharing" in this tree.** This component is sharing a URL _out_ to another app. Screen, camera and microphone sharing — the sharing indicator, "stop sharing" button, and per-tab sharing state — is WebRTC, lives in `browser/actors/WebRTCParent.sys.mjs`, and belongs to site permissions. A grep for `sharing` returns both, so check which one the report is actually about; a bug about an indicator or a "stop sharing" control is almost certainly the WebRTC one. + +## Tests + +browser-chrome under `browser/components/contentsharing/tests/browser/`, which has a `ContentSharingMockServer.sys.mjs` for the remote config — use it rather than stubbing the fetch yourself. Schema fixtures are xpcshell under `tests/unit/` (`validContentSharing.*.json` / `invalidContentSharing.*.json`), so a config-parsing bug has a very cheap regression test. The `widget/` half is effectively uncovered: there is no automated test for the macOS share sheet or the Windows share dialog, so for a platform-side bug say the area is untested rather than leaving the reader wondering. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/site-permissions.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/site-permissions.md new file mode 100644 index 0000000000..7e49c57e5b --- /dev/null +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/site-permissions.md @@ -0,0 +1,7 @@ +# Site permissions + +desktop JS, but split across the prompt, the state, and the store, so start by working out which of the three the bug is in. `browser/modules/SitePermissions.sys.mjs` holds the permission state the rest of the frontend reads and writes, including the defaults, the scopes (`SCOPE_PERSISTENT`, `SCOPE_SESSION`, `SCOPE_TEMPORARY`), and the `ALLOW`/`BLOCK`/`PROMPT` states. `browser/modules/PermissionUI.sys.mjs` builds the doorhanger prompts, one subclass per permission type. `browser/actors/WebRTCParent.sys.mjs` handles camera, microphone, and screen sharing, which do **not** go through the generic prompt path and carry their own sharing indicator. The management UI is `browser/components/preferences/dialogs/permissions.js` and `sitePermissions.js`. The backing store is `nsIPermissionManager`, implemented in C++ at `extensions/permissions/PermissionManager.cpp` — that is outside the frontend directories, so "the permission did not stick", "it came back after a restart", and wrong-expiry bugs are localized there and are **not** out of scope for being non-JS. + +## Tests + +the prompts are covered by browser-chrome under `browser/base/content/test/permissions/`, `SitePermissions.sys.mjs` itself by `browser/modules/test/browser/`, and the store by xpcshell under `extensions/permissions/test/`. Name the one that matches the layer you localized to, not whichever you found first. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/windows-installer.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/windows-installer.md new file mode 100644 index 0000000000..842d833de3 --- /dev/null +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/windows-installer.md @@ -0,0 +1,7 @@ +# Windows installer + +`browser/installer/windows/nsis/`. This is **NSIS**: `installer.nsi` (the full installer), `stub.nsi` (the small downloader stub), `uninstaller.nsi`, `maintenanceservice_installer.nsi`, and the `.nsh` include files that hold most of the logic. Localized strings live in the `.nsi`/`.properties` files alongside. The packaging manifests are `browser/installer/package-manifest.in` and `browser/installer/allowed-dupes.mn`, and the MSI and MSIX wrappers are in the sibling `msi/` and `msix/` directories. There is no JS here at all. Note which installer the bug is about: the stub and the full installer are separate programs with separate code. + +## Tests + +coverage is thin and specific. `browser/installer/windows/nsis/test/xpcshell/test_stub_installer.js` drives `test_stub.nsi` and covers the **stub** installer only; nothing exercises `installer.nsi` or the uninstaller. So for most Installer bugs an empty `relevant_tests` is the correct answer — say that the area is uncovered rather than leaving the reader to wonder whether you looked. diff --git a/agents/frontend-triage/tests/test_hooks.py b/agents/frontend-triage/tests/test_hooks.py index 56554feb4c..20534cc1cf 100644 --- a/agents/frontend-triage/tests/test_hooks.py +++ b/agents/frontend-triage/tests/test_hooks.py @@ -8,7 +8,11 @@ import pytest from agent_tools.registry import ToolError from hackbot_agents.frontend_triage.config import ENABLED_ACTION_TYPES -from hackbot_agents.frontend_triage.hooks import add_comment_hook, severity_block_hook +from hackbot_agents.frontend_triage.hooks import ( + add_comment_hook, + area_guidance_hook, + severity_block_hook, +) from hackbot_runtime.actions import ActionsRecorder from hackbot_runtime.actions.claude_sdk import actions_to_tool_names @@ -94,3 +98,66 @@ def test_a_comment_may_declare_its_severity_only_once(): with pytest.raises(ToolError): severity_block_hook({"params": {"text": plan + block + block}}) + + +def _cite(path: str) -> str: + """A comment citing ``path`` the way the prompt asks for it.""" + return f"The fault is in [{path}]({{{{searchfox.permalink}}}}/{path})." + + +def _area_hook(*loaded: str): + hook = area_guidance_hook(set(loaded)) + return lambda text: hook({"params": {"bug_id": BUG, "text": text}}) + + +def test_a_comment_citing_an_unloaded_area_is_refused(): + # The case the whole split has to survive: a New Tab Page bug that turns out to be + # the installer. Without this the run quietly fix-plans a tree it was told nothing + # about, which is worse than the prompt it replaced. + with pytest.raises(ToolError, match="Windows installer"): + _area_hook("Desktop frontend")( + _cite("browser/installer/windows/nsis/installer.nsi") + ) + + +def test_the_refusal_names_the_area_to_load(): + # The agent has to act on this in-run, so the message has to say which area and + # which tool -- "you are missing guidance" is not actionable. + with pytest.raises(ToolError) as e: + _area_hook("Desktop frontend")(_cite("mobile/android/fenix/Home.kt")) + assert "Firefox for Android" in str(e.value) + assert "load_area_guidance" in str(e.value) + + +def test_a_comment_citing_a_loaded_area_passes(): + _area_hook("Desktop frontend")(_cite("browser/components/tabbrowser/tabgroup.js")) + + +def test_an_area_loaded_mid_run_passes(): + # `load_area_guidance` adds to the same set the hook reads, so the retry after a + # refusal succeeds. If this ever decoupled, the agent would be stuck in a loop it + # cannot exit and the run would burn its turns. + loaded = {"Desktop frontend"} + hook = area_guidance_hook(loaded) + body = _cite("browser/installer/windows/nsis/installer.nsi") + with pytest.raises(ToolError): + hook({"params": {"bug_id": BUG, "text": body}}) + loaded.add("Windows installer") + hook({"params": {"bug_id": BUG, "text": body}}) + + +def test_a_comment_citing_no_known_area_passes(): + # A Graphics bug has no file to load, and it triages fine today off the source tree + # and Searchfox. Refusing here would fail the run over something the agent cannot + # satisfy -- it would retry forever against guidance that does not exist. + _area_hook("Desktop frontend")(_cite("gfx/thebes/gfxPlatform.cpp")) + + +def test_every_area_loaded_accepts_anything(): + # What an unknown component gets. Equivalent to the pre-split prompt, so the hook + # has to be inert for it. + from hackbot_agents.frontend_triage.config import AREAS + + hook = _area_hook(*(a.name for a in AREAS)) + hook(_cite("browser/installer/windows/nsis/installer.nsi")) + hook(_cite("toolkit/mozapps/update/UpdateService.sys.mjs")) diff --git a/agents/frontend-triage/tests/test_plan.py b/agents/frontend-triage/tests/test_plan.py index 191c369722..cd6cb26a4f 100644 --- a/agents/frontend-triage/tests/test_plan.py +++ b/agents/frontend-triage/tests/test_plan.py @@ -7,6 +7,7 @@ from pathlib import Path from hackbot_agents.frontend_triage.agent import ( + AREAS_DIR, load_system_prompt, may_apply_unattended, parse_bug_id, @@ -17,7 +18,13 @@ parse_severity_assessment, render_scope, ) -from hackbot_agents.frontend_triage.config import TRIAGE_SCOPE, ScopedComponent +from hackbot_agents.frontend_triage.config import ( + AREAS, + TRIAGE_SCOPE, + ScopedComponent, + area_for_path, + areas_for, +) def _block(body: str) -> str: @@ -28,7 +35,7 @@ def test_the_system_prompt_renders(): # system.md goes through str.format, so a literal brace in it must be doubled or # startup raises KeyError and the run never begins. The structured-output block is # where that happens. - prompt = load_system_prompt(Path("rules"), "") + prompt = load_system_prompt(Path("rules"), "", areas_for("Firefox", "New Tab Page")) assert '"severity_assessment": {' in prompt assert "{rules_dir}" not in prompt assert "{triaged_components}" not in prompt @@ -64,16 +71,71 @@ def test_the_scope_says_it_is_neither_a_limit_nor_a_vocabulary(): assert "verbatim" in rendered -def test_every_area_has_prompt_guidance(): +def test_every_area_has_a_guidance_file(): # The registry is what makes a component triaged; this is what makes it triageable. - # `Source repository` carries the per-area code layout, and an area with no bullet - # there means the agent is pointed at a component with no idea where its code lives - # -- which is how a bug gets read as out of scope and skipped. So a new area costs - # two files, visibly, rather than one file plus a prompt nobody remembered. - prompt = load_system_prompt(Path("rules"), "") - source_section = prompt.split("# Source repository", 1)[1] - for area in {entry.area for entry in TRIAGE_SCOPE}: - assert f"**{area}**" in source_section, area + # An area whose file is missing points the agent at a component with no idea where + # its code lives -- which is how a bug gets read as out of scope and skipped. So a + # new area costs two files, visibly, rather than one file plus a prompt nobody + # remembered. + for area in AREAS: + assert (AREAS_DIR / f"{area.slug}.md").is_file(), area.name + + +def test_every_registry_area_resolves(): + # `area` and `related_areas` are strings, so a typo in either is only caught here. + # `areas_for` would raise KeyError mid-run, after the bug was already fetched. + names = {a.name for a in AREAS} + for entry in TRIAGE_SCOPE: + assert entry.area in names, entry.key + for related in entry.related_areas: + assert related in names, f"{entry.key} -> {related}" + + +def test_an_unknown_component_gets_every_area(): + # `rules/scoping.md` puts an unlisted component in scope, so guessing one area for + # it would leave the run with less than it has today. Failing open costs the old + # prompt size and nothing else. + assert areas_for("Firefox", "Graphics") == AREAS + assert areas_for(None, None) == AREAS + + +def test_a_component_with_a_known_overlap_gets_both_areas(): + # A "stop sharing" report arrives under Sharing but is WebRTC, which site + # permissions owns. Both files ship from the start rather than the agent having to + # notice mid-run -- see `ScopedComponent.related_areas`. + assert [a.name for a in areas_for("Firefox", "Sharing")] == [ + "Sharing", + "Site permissions", + ] + + +def test_only_the_matching_area_reaches_the_prompt(): + # The point of the split. Everything else stays reachable via the index and + # `load_area_guidance`, but its text is not paid for on every run. + prompt = load_system_prompt(Path("rules"), "", areas_for("Firefox", "New Tab Page")) + assert "NSIS" not in prompt + assert "IPProtectionPanel.sys.mjs" not in prompt + # ...while the index still names every area, so a mislocalized bug is recognisable. + for area in AREAS: + assert f"**{area.name}**" in prompt, area.name + + +def test_a_nested_area_wins_over_the_tree_containing_it(): + # `browser/` is the desktop frontend and `browser/installer/` is not, so a plain + # prefix scan in registry order would file every installer bug under the wrong area + # and the guidance hook would never fire. + assert area_for_path("browser/installer/windows/nsis/stub.nsi").name == ( + "Windows installer" + ) + assert area_for_path("browser/components/tabbrowser/tabgroup.js").name == ( + "Desktop frontend" + ) + + +def test_a_path_in_no_area_belongs_to_no_area(): + # Load-bearing for `area_guidance_hook`: None means "no guidance exists", not + # "guidance is missing", and must not be treated as something the agent can fetch. + assert area_for_path("gfx/thebes/gfxPlatform.cpp") is None def test_confidence_is_normalized(): From c902b6adc7676f14883a416bd55944c24525a50b Mon Sep 17 00:00:00 2001 From: Cory Thomas Date: Fri, 21 Aug 2026 20:51:56 -0400 Subject: [PATCH 2/7] frontend-triage: stop stating the duplicate reporting rules twice The system prompt and the duplicate-detection ruleset both said the verdict never gates the triage, what the comment line looks like, and to fill in duplicate_assessment either way -- in different words, so the two could drift apart without either looking wrong. The system prompt keeps all three. It is read on every run, and the never-gate rule in particular is a safety property that cannot sit in a file the agent decides whether to open. Recording actions already says the stronger version of "you have no tool that changes a field", resolution included, so the ruleset was the third place that appeared. What is left in the ruleset is what a ruleset is for: what makes two reports the same defect, the per-area signals, and the bar for naming one at all. --- .../rules/duplicate-detection.md | 24 ++++++------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/duplicate-detection.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/duplicate-detection.md index 12f90f75d0..b73e5343f3 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/duplicate-detection.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/duplicate-detection.md @@ -1,12 +1,9 @@ # Duplicate detection -These rules apply to **every** bug: the `duplicate_hunter` subagent runs before -localization and asks one question — is this already filed? - -The verdict is a **suggestion only**. Naming a duplicate never stops the triage: the fix -plan is written either way, `actionable` stays `true`, and confidence is unaffected. A -human decides whether the bugs are really the same and marks them. The agent cannot mark -one — it has no tool that changes a Bugzilla field. +These rules apply to **every** bug. They are the judgment behind the verdict the +`duplicate_hunter` returns — what makes two reports the same defect, and when to keep +quiet. Spawning it, reporting the verdict and recording it are all in the system prompt, +and are not repeated here. ## What counts as the same defect @@ -39,14 +36,7 @@ does: - **IP Protection.** Whether the proxy state is _displayed_ wrong or _actually_ wrong. Those are different defects with different severities and should not be merged. -## Reporting - -- **Candidate found** — the triage comment opens with `**Possible duplicate:** ` and - nothing else about it. No hedging, no explanation; the reader opens the bug and judges. -- **Nothing found** — say nothing. The comment opens with the analysis as usual. Absence is - not reported. -- **Uncertain** — prefer reporting nothing. A wrong duplicate sends someone to the wrong - bug and costs more trust than a missed one costs time. +## The bar for naming one -Record the same verdict in the `duplicate_assessment` structured output either way, so a -run that found nothing is distinguishable from one that never looked. +Prefer reporting nothing when uncertain. A wrong duplicate sends someone to the wrong bug +and costs more trust than a missed one costs time. From 6263ac871286aa66facc0a916773bbebc159bcf2 Mon Sep 17 00:00:00 2001 From: Cory Thomas Date: Fri, 21 Aug 2026 20:57:59 -0400 Subject: [PATCH 3/7] frontend-triage: only enforce area guidance on paths an area exclusively owns The guidance hook refused comments the guidance itself had asked for. Desktop frontend was listed as owning browser/, toolkit/ and devtools/, which contain most of the other areas, so an IP Protection bug citing browser/app/profile/firefox.js for its prefs -- a path rules/areas/ip-protection.md names -- came back refused. Six of eight realistic citations hit it: ordinary desktop chrome, an area's own test directory, a widget/ file from a bug that had nothing to do with sharing. Each one would have cost a run two turns recovering from a refusal it could not have avoided. Ownership is now separate from the index. `trees` stays descriptive and may overlap; `owns` is what the hook reads and has to mean "no other area could mean this file". Desktop frontend owns nothing, being the general case, and its guidance is the two lines it can least afford to lose. The narrow areas -- installer, Android, updater, IP Protection -- keep theirs, so the hook still fires where the guidance is worth enforcing. The regression test is per-component rather than per-area: every path an area's guidance names must survive the hook for every component that loads it, related areas included. That is what makes Sharing's reference to WebRTCParent legal, and it fails if the broad trees are ever marked owned again. --- .../hackbot_agents/frontend_triage/config.py | 80 +++++++++++++++---- agents/frontend-triage/tests/test_plan.py | 60 ++++++++++++-- 2 files changed, 117 insertions(+), 23 deletions(-) diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py index 072b4c5705..4d9196c2f7 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py @@ -81,15 +81,26 @@ class Area(NamedTuple): name: str slug: str - # Path prefixes this area owns. Longest match wins in `area_for_path`, so a nested - # area resolves ahead of the tree containing it -- `browser/installer/` is the - # Windows installer, not the desktop frontend's `browser/`. + # Where this area's code lives, for the prompt's index. Descriptive, and allowed to + # be broad and to overlap another area: `browser/` names the desktop frontend + # usefully even though most other areas sit inside it. trees: tuple[str, ...] + # Paths this area **exclusively** owns, for `area_for_path` and so for + # `hooks.area_guidance_hook`. Narrower than `trees` on purpose -- enforcement needs + # "no other area could mean this file", and `browser/` fails that badly enough to + # refuse comments the guidance itself asked for: `rules/areas/ip-protection.md` + # sends the agent to `browser/app/profile/firefox.js` for prefs. + # + # Empty for the desktop frontend, which is the general case and owns nothing + # exclusively. It costs the least to leave unenforced -- its guidance is two lines, + # against the installer's NSIS or Android's Kotlin. + owns: tuple[str, ...] = () # Every area, in the order they are listed to the model. `slug` is the filename under # `rules/areas/`; `trees` drives both that index and `hooks.area_guidance_hook`. AREAS = ( + # No `owns`: everything below sits inside these trees. Area("Desktop frontend", "desktop-frontend", ("browser/", "toolkit/", "devtools/")), Area( "Site permissions", @@ -98,7 +109,12 @@ class Area(NamedTuple): "browser/modules/SitePermissions.sys.mjs", "browser/modules/PermissionUI.sys.mjs", "browser/actors/WebRTCParent.sys.mjs", - "browser/components/preferences/dialogs/", + "extensions/permissions/", + ), + owns=( + "browser/modules/SitePermissions.sys.mjs", + "browser/modules/PermissionUI.sys.mjs", + "browser/actors/WebRTCParent.sys.mjs", "extensions/permissions/", ), ), @@ -108,13 +124,22 @@ class Area(NamedTuple): ( "browser/modules/SharingUtils.sys.mjs", "browser/components/contentsharing/", - "widget/", + "widget/ (the per-OS half)", + ), + # Not `widget/`: that is the whole platform widget layer, and a bug in any + # other area citing a file there has nothing to do with sharing a URL out. + owns=( + "browser/modules/SharingUtils.sys.mjs", + "browser/components/contentsharing/", + "widget/nsIMacSharingService.idl", + "widget/cocoa/nsMacSharingService.mm", ), ), Area( "IP Protection", "ip-protection", ("browser/components/ipprotection/", "toolkit/components/ipprotection/"), + owns=("browser/components/ipprotection/", "toolkit/components/ipprotection/"), ), Area( "Messaging System", @@ -124,10 +149,30 @@ class Area(NamedTuple): "browser/components/aboutwelcome/", "toolkit/components/messaging-system/", ), + owns=( + "browser/components/asrouter/", + "browser/components/aboutwelcome/", + "toolkit/components/messaging-system/", + ), + ), + Area( + "Firefox for Android", + "firefox-for-android", + ("mobile/android/",), + owns=("mobile/android/",), + ), + Area( + "Application updater", + "application-updater", + ("toolkit/mozapps/update/",), + owns=("toolkit/mozapps/update/",), + ), + Area( + "Windows installer", + "windows-installer", + ("browser/installer/",), + owns=("browser/installer/",), ), - Area("Firefox for Android", "firefox-for-android", ("mobile/android/",)), - Area("Application updater", "application-updater", ("toolkit/mozapps/update/",)), - Area("Windows installer", "windows-installer", ("browser/installer/",)), ) AREAS_BY_NAME = {a.name: a for a in AREAS} @@ -222,18 +267,21 @@ def areas_for(product: str | None, component: str | None) -> tuple[Area, ...]: def area_for_path(path: str) -> Area | None: - """The area owning ``path``, or None if no area does. + """The area that exclusively owns ``path``, or None if none does. + + None is the common and correct answer, not a failure. It covers a file outside the + triaged areas (`gfx/`) and any ordinary desktop chrome file (`browser/base/...`), + which no area owns exclusively -- see `Area.owns`. Callers must read it as "no + guidance is specific to this file", never as "guidance is missing". - None is the ordinary answer for a file outside the triaged areas -- `gfx/`, say -- - and it means "no guidance exists for this", not "guidance is missing". Longest - prefix wins so `browser/installer/...` resolves to the installer rather than to the - desktop frontend's `browser/`. + Longest match wins, so `browser/installer/...` is the installer even though the + desktop frontend describes `browser/`. """ best: tuple[int, Area] | None = None for area in AREAS: - for tree in area.trees: - if path.startswith(tree) and (best is None or len(tree) > best[0]): - best = (len(tree), area) + for owned in area.owns: + if path.startswith(owned) and (best is None or len(owned) > best[0]): + best = (len(owned), area) return best[1] if best else None diff --git a/agents/frontend-triage/tests/test_plan.py b/agents/frontend-triage/tests/test_plan.py index cd6cb26a4f..d511bdb42d 100644 --- a/agents/frontend-triage/tests/test_plan.py +++ b/agents/frontend-triage/tests/test_plan.py @@ -4,6 +4,7 @@ with nobody in between, so it is covered as closely as the hooks are. """ +import re from pathlib import Path from hackbot_agents.frontend_triage.agent import ( @@ -120,16 +121,16 @@ def test_only_the_matching_area_reaches_the_prompt(): assert f"**{area.name}**" in prompt, area.name -def test_a_nested_area_wins_over_the_tree_containing_it(): - # `browser/` is the desktop frontend and `browser/installer/` is not, so a plain - # prefix scan in registry order would file every installer bug under the wrong area - # and the guidance hook would never fire. +def test_an_owned_subtree_resolves_even_though_a_broader_area_describes_it(): + # The desktop frontend's index entry covers `browser/`, but the installer and IP + # Protection sit inside it and own their own subtrees. Ownership has to follow the + # specific claim, or the hook never fires for the areas whose guidance matters most. assert area_for_path("browser/installer/windows/nsis/stub.nsi").name == ( "Windows installer" ) - assert area_for_path("browser/components/tabbrowser/tabgroup.js").name == ( - "Desktop frontend" - ) + assert area_for_path( + "browser/components/ipprotection/IPProtection.sys.mjs" + ).name == ("IP Protection") def test_a_path_in_no_area_belongs_to_no_area(): @@ -336,3 +337,48 @@ def test_a_duplicate_verdict_does_not_change_what_reaches_a_bug(): assert may_apply_unattended(base) assert may_apply_unattended(found) assert may_apply_unattended(none_found) + + +# Paths written as `some/dir/File.ext` in an area's guidance prose. +_GUIDANCE_PATH = re.compile(r"`([a-z][a-z0-9_./-]*/[A-Za-z0-9_./-]+)`") + + +def test_guidance_never_names_a_path_its_own_component_cannot_cite(): + # The invariant that keeps `area_guidance_hook` honest, and the one that caught + # `browser/` being listed as owned: an area told the agent where the prefs and + # strings were, and citing them then had the comment refused. Every path a + # component's own guidance names has to survive the hook for that component -- + # including across `related_areas`, which is what makes Sharing's reference to + # WebRTCParent legal. + for entry in TRIAGE_SCOPE: + areas = areas_for(entry.product, entry.component) + loaded = {a.name for a in areas} + for area in areas: + text = (AREAS_DIR / f"{area.slug}.md").read_text() + for match in _GUIDANCE_PATH.finditer(text): + owner = area_for_path(match.group(1)) + assert owner is None or owner.name in loaded, ( + f"{entry.key}: guidance names {match.group(1)}, " + f"owned by {owner.name if owner else None}" + ) + + +def test_ordinary_desktop_chrome_is_owned_by_nobody(): + # `browser/` and `toolkit/` describe the desktop frontend usefully in the index but + # contain almost every other area, so treating them as owned refuses comments for + # the ordinary reason that a Firefox bug touches a Firefox file. + for path in ( + "browser/base/content/browser.js", + "browser/app/profile/firefox.js", + "toolkit/content/widgets/panel-list.js", + "widget/cocoa/nsCocoaWindow.mm", + ): + assert area_for_path(path) is None, path + + +def test_areas_with_real_guidance_still_own_something(): + # The other half: if `owns` were emptied to silence false positives the hook would + # never fire at all. Desktop frontend is the deliberate exception -- it is the + # general case, and its guidance is two lines. + unenforced = [a.name for a in AREAS if not a.owns] + assert unenforced == ["Desktop frontend"], unenforced From b0ffc0716990b1cd94d84ca086cd1311afb61981 Mon Sep 17 00:00:00 2001 From: Cory Thomas Date: Fri, 21 Aug 2026 21:03:45 -0400 Subject: [PATCH 4/7] frontend-triage: trim the area-split comments and tests Four tests removed. Two restated the registry rather than testing behaviour, and two were covered by the per-component guidance-path invariant, which fails on the same defects -- checked by reintroducing each one and watching what broke. The docstrings said the same thing two or three ways. Kept the reasons that are not readable off the code: why the lookup happens before the run rather than at step 1, why `owns` is narrower than `trees`, why a path in no area passes, and why headings are demoted on the way into the prompt. Also two stale references the split left behind: render_scope still described the guidance as living under `Source repository`, and the AREAS comment still credited `trees` with driving the hook. --- .../hackbot_agents/frontend_triage/agent.py | 30 ++++++------- .../hackbot_agents/frontend_triage/areas.py | 14 +++--- .../hackbot_agents/frontend_triage/config.py | 43 ++++++++----------- .../hackbot_agents/frontend_triage/hooks.py | 15 +++---- agents/frontend-triage/tests/test_hooks.py | 26 ++--------- agents/frontend-triage/tests/test_plan.py | 18 -------- 6 files changed, 44 insertions(+), 102 deletions(-) diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py b/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py index c3de0ff5e0..8a848444f8 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py @@ -168,8 +168,8 @@ def render_scope(scope: tuple[ScopedComponent, ...] = TRIAGE_SCOPE) -> str: """Render `config.TRIAGE_SCOPE` as the prompt's component list, grouped by area. Generated rather than written into the prompt so that the component list has one - home. The per-area guidance under `Source repository` stays hand-authored: it is - prose about a codebase, and only the enumeration is mechanical. + home. The `rules/areas/` guidance stays hand-authored: it is prose about a + codebase, and only the enumeration is mechanical. Takes the registry as an argument so a test can assert the grouping against a fixed input rather than against whatever the real scope happens to be today. @@ -207,16 +207,12 @@ async def fetch_product_component( ) -> tuple[str | None, str | None]: """The bug's product and component, read through the Bugzilla broker. - Needed before the agent starts, because the system prompt is built once and the - area guidance goes into it -- see `areas_for`. The agent's own first step fetches - the bug too, but that is several turns after the prompt is frozen. + The agent fetches the bug itself at step 1, but the prompt is built and frozen + before that, and the area guidance goes into it. Via the broker because the agent + container binds no Bugzilla credentials (see `compose.yml`). - Goes through the broker's MCP endpoint because that is the only Bugzilla path this - process has: the agent container binds no credentials (see `compose.yml`). - - Returns ``(None, None)`` on any failure, which `areas_for` turns into every area -- - today's prompt. Never raises: the guidance is an optimization, and a broken lookup - must not take down a run that would otherwise work. + Never raises. ``(None, None)`` makes `areas_for` send every area, which is the + prompt this replaced -- a broken lookup must not take down a workable run. """ url = ( bugzilla_mcp_server.get("url") @@ -251,9 +247,8 @@ async def fetch_product_component( def render_area_index() -> str: """One line per area: its name and the trees it covers. - Always in the prompt, even when only one area's guidance is. It is what lets the - agent recognise that the code it just localized into belongs to an area it does not - have, which is the trigger for `load_area_guidance`. + Always in the prompt, even when one area's guidance is, so the agent can recognise + that it has localized into an area it does not have. """ return "\n".join(f"- **{a.name}** — {', '.join(a.trees)}" for a in AREAS) @@ -261,10 +256,9 @@ def render_area_index() -> str: def read_area_guidance(areas: Sequence[Area]) -> str: """The `rules/areas/` files for ``areas``, concatenated for the prompt. - Headings are demoted two levels on the way in. The files are `# ` because - `load_area_guidance` serves them whole, but pasted verbatim that H1 would sit - between `# Source repository` and `# Linking source files` and read as a new - top-level section rather than as part of the one it belongs to. + Headings drop two levels on the way in. The files are `# ` because + `load_area_guidance` serves them whole, but that H1 pasted between + `# Source repository` and `# Linking source files` reads as a new section. """ bodies = [] for area in areas: diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/areas.py b/agents/frontend-triage/hackbot_agents/frontend_triage/areas.py index c886f3aa58..3ae27cb673 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/areas.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/areas.py @@ -1,12 +1,8 @@ """The ``load_area_guidance`` tool -- read a `rules/areas/` file mid-run. -The prompt carries the guidance for the area the bug's component maps to. This is how -the agent gets a *different* one when its investigation says the code lives elsewhere, -and it records what was loaded so `hooks.area_guidance_hook` can tell whether a comment -is citing a tree the agent was told nothing about. - -A plain ``Read`` of the file would work equally well for the agent and not at all for -the hook, which needs the load to be observable. +How the agent gets an area other than the one its component mapped to. A plain ``Read`` +would serve the agent equally well and `hooks.area_guidance_hook` not at all, which +needs the load to be observable. """ from __future__ import annotations @@ -25,8 +21,8 @@ class AreaGuidanceContext: """Where the area files live, and which ones this run has loaded. - ``loaded`` is shared with the hook rather than copied: it starts as the areas that - were injected into the prompt, and each successful call adds to it. + ``loaded`` is shared with the hook rather than copied, and starts as whatever the + prompt was built with. """ areas_dir: Path diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py index 4d9196c2f7..8530cd10f6 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py @@ -64,11 +64,10 @@ class ScopedComponent(NamedTuple): # triage with nobody told -- which is what `channel_for` failing closed produces, # and not something to be able to express by accident. channel: str - # Areas whose guidance goes in the prompt alongside `area`, for components that - # routinely turn out to be somewhere else. Sharing is the known one: a "stop - # sharing" report arrives here but is WebRTC, which site permissions owns. Listing - # the pair means both files are present from the start, rather than the agent - # having to notice mid-run and fetch the second one. + # Areas sent alongside `area`, for components that routinely turn out to be + # somewhere else: a "stop sharing" report arrives under Sharing but is WebRTC, + # which site permissions owns. Both ship from the start rather than the agent + # having to notice mid-run. related_areas: tuple[str, ...] = () @property @@ -81,24 +80,20 @@ class Area(NamedTuple): name: str slug: str - # Where this area's code lives, for the prompt's index. Descriptive, and allowed to - # be broad and to overlap another area: `browser/` names the desktop frontend - # usefully even though most other areas sit inside it. + # Where this area's code lives, for the prompt's index. Descriptive, so it may be + # broad and overlap another area. trees: tuple[str, ...] # Paths this area **exclusively** owns, for `area_for_path` and so for - # `hooks.area_guidance_hook`. Narrower than `trees` on purpose -- enforcement needs + # `hooks.area_guidance_hook`. Deliberately narrower than `trees`: enforcement needs # "no other area could mean this file", and `browser/` fails that badly enough to - # refuse comments the guidance itself asked for: `rules/areas/ip-protection.md` - # sends the agent to `browser/app/profile/firefox.js` for prefs. - # - # Empty for the desktop frontend, which is the general case and owns nothing - # exclusively. It costs the least to leave unenforced -- its guidance is two lines, - # against the installer's NSIS or Android's Kotlin. + # refuse comments the guidance asked for -- `rules/areas/ip-protection.md` sends + # the agent to `browser/app/profile/firefox.js` for prefs. Empty for the desktop + # frontend, the general case, which owns nothing exclusively. owns: tuple[str, ...] = () # Every area, in the order they are listed to the model. `slug` is the filename under -# `rules/areas/`; `trees` drives both that index and `hooks.area_guidance_hook`. +# `rules/areas/`. AREAS = ( # No `owns`: everything below sits inside these trees. Area("Desktop frontend", "desktop-frontend", ("browser/", "toolkit/", "devtools/")), @@ -252,11 +247,10 @@ class Area(NamedTuple): def areas_for(product: str | None, component: str | None) -> tuple[Area, ...]: """The areas whose guidance belongs in the prompt for a bug in this component. - **Every** area when the component is not one we triage, or when the caller could - not determine it. `rules/scoping.md` is explicit that a defect in an unlisted - component is still in scope, and a run that guessed one area for such a bug would - have less to work with than it does today. Failing open costs the current prompt - size and nothing else, so it is the only safe default. + **Every** area for a component we do not triage, or one the caller could not + determine. `rules/scoping.md` puts an unlisted component in scope, so guessing one + area would leave those runs with less than they have today; failing open costs only + the prompt size it already had. """ entry = _SCOPE_BY_KEY.get( f"{(product or '').strip()} :: {(component or '').strip()}" @@ -269,10 +263,9 @@ def areas_for(product: str | None, component: str | None) -> tuple[Area, ...]: def area_for_path(path: str) -> Area | None: """The area that exclusively owns ``path``, or None if none does. - None is the common and correct answer, not a failure. It covers a file outside the - triaged areas (`gfx/`) and any ordinary desktop chrome file (`browser/base/...`), - which no area owns exclusively -- see `Area.owns`. Callers must read it as "no - guidance is specific to this file", never as "guidance is missing". + None is the common and correct answer -- it covers both a file outside the triaged + areas (`gfx/`) and ordinary desktop chrome. Read it as "no guidance is specific to + this file", never as "guidance is missing". Longest match wins, so `browser/installer/...` is the installer even though the desktop frontend describes `browser/`. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/hooks.py b/agents/frontend-triage/hackbot_agents/frontend_triage/hooks.py index 848baff363..522cfc27e2 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/hooks.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/hooks.py @@ -62,19 +62,14 @@ def cited_paths(text: str) -> list[str]: def area_guidance_hook(loaded_areas: set[str]) -> ActionHook: """Refuse a comment citing code from an area whose guidance was never loaded. - The prompt carries one area's guidance, chosen from the bug's component before the - run starts. When the investigation lands somewhere else -- a New Tab Page bug that - turns out to be the installer -- the agent has to call `load_area_guidance` for it, - and nothing but this hook makes that reliable. Without it the run would quietly - fix-plan a tree it was told nothing about, which is worse than what it does today - with every area in the prompt. + The prompt carries the area the bug's component maps to. When the investigation + lands somewhere else, nothing but this makes the agent go and read that area. ``loaded_areas`` is shared with the ``areas`` MCP server, which adds to it as the - agent loads files; it starts as whatever was injected. + agent loads files, so the retry after a refusal succeeds. - A path in **no** area passes. `gfx/` has no guidance to load, and that case works - today -- the agent localizes from the tree and Searchfox. Blocking on it would fail - a run over something the agent cannot possibly satisfy. + A path in no area passes: `gfx/` has nothing to load, so refusing it would fail the + run over something the agent cannot satisfy. """ def hook(action: dict) -> None: diff --git a/agents/frontend-triage/tests/test_hooks.py b/agents/frontend-triage/tests/test_hooks.py index 20534cc1cf..cb8849b918 100644 --- a/agents/frontend-triage/tests/test_hooks.py +++ b/agents/frontend-triage/tests/test_hooks.py @@ -111,21 +111,13 @@ def _area_hook(*loaded: str): def test_a_comment_citing_an_unloaded_area_is_refused(): - # The case the whole split has to survive: a New Tab Page bug that turns out to be - # the installer. Without this the run quietly fix-plans a tree it was told nothing - # about, which is worse than the prompt it replaced. - with pytest.raises(ToolError, match="Windows installer"): + # A New Tab Page bug that turns out to be the installer. The agent acts on this + # in-run, so the message has to name the area and the tool. + with pytest.raises(ToolError) as e: _area_hook("Desktop frontend")( _cite("browser/installer/windows/nsis/installer.nsi") ) - - -def test_the_refusal_names_the_area_to_load(): - # The agent has to act on this in-run, so the message has to say which area and - # which tool -- "you are missing guidance" is not actionable. - with pytest.raises(ToolError) as e: - _area_hook("Desktop frontend")(_cite("mobile/android/fenix/Home.kt")) - assert "Firefox for Android" in str(e.value) + assert "Windows installer" in str(e.value) assert "load_area_guidance" in str(e.value) @@ -151,13 +143,3 @@ def test_a_comment_citing_no_known_area_passes(): # and Searchfox. Refusing here would fail the run over something the agent cannot # satisfy -- it would retry forever against guidance that does not exist. _area_hook("Desktop frontend")(_cite("gfx/thebes/gfxPlatform.cpp")) - - -def test_every_area_loaded_accepts_anything(): - # What an unknown component gets. Equivalent to the pre-split prompt, so the hook - # has to be inert for it. - from hackbot_agents.frontend_triage.config import AREAS - - hook = _area_hook(*(a.name for a in AREAS)) - hook(_cite("browser/installer/windows/nsis/installer.nsi")) - hook(_cite("toolkit/mozapps/update/UpdateService.sys.mjs")) diff --git a/agents/frontend-triage/tests/test_plan.py b/agents/frontend-triage/tests/test_plan.py index d511bdb42d..074941a8eb 100644 --- a/agents/frontend-triage/tests/test_plan.py +++ b/agents/frontend-triage/tests/test_plan.py @@ -100,16 +100,6 @@ def test_an_unknown_component_gets_every_area(): assert areas_for(None, None) == AREAS -def test_a_component_with_a_known_overlap_gets_both_areas(): - # A "stop sharing" report arrives under Sharing but is WebRTC, which site - # permissions owns. Both files ship from the start rather than the agent having to - # notice mid-run -- see `ScopedComponent.related_areas`. - assert [a.name for a in areas_for("Firefox", "Sharing")] == [ - "Sharing", - "Site permissions", - ] - - def test_only_the_matching_area_reaches_the_prompt(): # The point of the split. Everything else stays reachable via the index and # `load_area_guidance`, but its text is not paid for on every run. @@ -374,11 +364,3 @@ def test_ordinary_desktop_chrome_is_owned_by_nobody(): "widget/cocoa/nsCocoaWindow.mm", ): assert area_for_path(path) is None, path - - -def test_areas_with_real_guidance_still_own_something(): - # The other half: if `owns` were emptied to silence false positives the hook would - # never fire at all. Desktop frontend is the deliberate exception -- it is the - # general case, and its guidance is two lines. - unenforced = [a.name for a in AREAS if not a.owns] - assert unenforced == ["Desktop frontend"], unenforced From 242bb5cbb3ae72d3444e4b1d66c03cbf35088146 Mon Sep 17 00:00:00 2001 From: Cory Thomas Date: Fri, 21 Aug 2026 22:09:39 -0400 Subject: [PATCH 5/7] frontend-triage: ask for `id` in the component lookup The lookup asked for `product,component`, and `get_bugs` diffs the ids it requested against the ones it got back to report inaccessible bugs -- so leaving `id` out of include_fields made the tool itself raise KeyError('id'). Every run fell back to sending all eight areas. It failed safely, which is the design working: a real run against Bugzilla still produced a good triage, just with the prompt it had before this branch. But it failed silently, because a tool-side error arrived here as a JSON parse failure with the real message discarded. Check isError and put the message in the log. Confirmed against a live broker: the lookup now resolves, and a New Tab Page bug gets Desktop frontend alone. --- .../hackbot_agents/frontend_triage/agent.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py b/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py index 8a848444f8..dd70a9a850 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py @@ -228,10 +228,16 @@ async def fetch_product_component( await session.initialize() res = await session.call_tool( "get_bugs", - {"ids": [bug], "include_fields": "product,component"}, + # `id` is not optional: `get_bugs` diffs requested against + # returned ids to report inaccessible bugs, so leaving it out + # of `include_fields` makes the tool itself raise KeyError. + {"ids": [bug], "include_fields": "id,product,component"}, ) - payload = json.loads(res.content[0].text) - bugs = payload.get("bugs") or [] + if res.isError: + raise RuntimeError( + "".join(getattr(c, "text", "") for c in res.content) + ) + bugs = json.loads(res.content[0].text).get("bugs") or [] if not bugs: return None, None return bugs[0].get("product"), bugs[0].get("component") From 8ccb07116170ef9e92eb27168fc499b2b2d72afd Mon Sep 17 00:00:00 2001 From: Cory Thomas Date: Fri, 21 Aug 2026 22:17:24 -0400 Subject: [PATCH 6/7] frontend-triage: point the Sharing area at the paths that exist `browser/components/contentsharing/` and `browser/modules/SharingUtils.sys.mjs` are both gone -- the tree is `browser/components/sharing/` now, with SharingUtils alongside ContentSharingUtils inside it. The names came from the prompt prose, which has been stale since before this branch (master names them too), and I took the ownership prefixes from that prose instead of checking the checkout. The effect was that Sharing owned nothing real, so the guidance hook could never fire for it. A live run on bug 2040869 localized into browser/components/sharing/content/content-sharing-modal.mjs and the hook stayed silent; those three paths resolve to Sharing now. The prose in rules/areas/sharing.md still names the old paths. That is the pre-existing bug and wants its own change -- this is only the ownership list. --- .../hackbot_agents/frontend_triage/config.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py index 8530cd10f6..02e8abc89b 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py @@ -116,16 +116,11 @@ class Area(NamedTuple): Area( "Sharing", "sharing", - ( - "browser/modules/SharingUtils.sys.mjs", - "browser/components/contentsharing/", - "widget/ (the per-OS half)", - ), + ("browser/components/sharing/", "widget/ (the per-OS half)"), # Not `widget/`: that is the whole platform widget layer, and a bug in any # other area citing a file there has nothing to do with sharing a URL out. owns=( - "browser/modules/SharingUtils.sys.mjs", - "browser/components/contentsharing/", + "browser/components/sharing/", "widget/nsIMacSharingService.idl", "widget/cocoa/nsMacSharingService.mm", ), From 9698ac9011c4b052bcea5a546dfc73bce97202b8 Mon Sep 17 00:00:00 2001 From: Cory Thomas Date: Fri, 21 Aug 2026 22:31:10 -0400 Subject: [PATCH 7/7] frontend-triage: correct the Sharing paths in the guidance prose The sharing tree was reorganised and the prompt was never updated -- these names have been wrong since before this branch. Everything is under browser/components/sharing/ now: browser/modules/SharingUtils.sys.mjs -> browser/components/sharing/ browser/components/contentsharing/ -> browser/components/sharing/ .../contentsharing/tests/browser/ -> .../sharing/tests/browser/ tests/unit/ (bare, ambiguous) -> .../sharing/tests/unit/ SharingUtils and ContentSharingUtils sit in one directory now rather than two, so the sentence introducing the second one says "alongside it" instead of naming a separate tree. Wording is otherwise untouched. Every path in the file now resolves against a current checkout. Checked the other seven area files the same way: the paths they name are relative-to-context fragments and all resolve. --- .../hackbot_agents/frontend_triage/rules/areas/sharing.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/sharing.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/sharing.md index c650b8e214..caeebf856e 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/sharing.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/areas/sharing.md @@ -1,9 +1,9 @@ # Sharing -sending the current page to another app, and the one area here whose code reaches outside `browser/`, `toolkit/` and `devtools/`. `browser/modules/SharingUtils.sys.mjs` is the frontend: it populates the share menu, gates on `BrowserUtils.getShareableURL` (which is why an unshareable scheme silently yields no menu item), and then hands off to the platform. `browser/components/contentsharing/` is the newer piece — `ContentSharingUtils.sys.mjs`, the remotely-delivered config validated against `contentsharing.schema.json`, `content/`, and its own `metrics.yaml`. **The platform half is in `widget/`**, which the desktop frontend trees (`browser/`, `toolkit/`, `devtools/`) do not cover: `widget/nsIMacSharingService.idl` with `widget/cocoa/nsMacSharingService.mm` (Objective-C++ — the macOS share sheet, `getSharingProviders`, `openSharingPreferences`), and `widget/nsIWindowsUIUtils.idl`'s `shareUrl` for Windows. So "the Share menu is empty", "the wrong apps are listed", and "Share does nothing" are usually localized in `widget/`, per-OS, and are **not** out of scope for being C++ rather than JS. Note which OS the bug is about before reading either. +sending the current page to another app, and the one area here whose code reaches outside `browser/`, `toolkit/` and `devtools/`. `browser/components/sharing/` holds all of it. `SharingUtils.sys.mjs` there is the frontend: it populates the share menu, gates on `BrowserUtils.getShareableURL` (which is why an unshareable scheme silently yields no menu item), and then hands off to the platform. Alongside it, `ContentSharingUtils.sys.mjs` is the newer piece — the remotely-delivered config validated against `contentsharing.schema.json`, `content/`, and its own `metrics.yaml`. **The platform half is in `widget/`**, which the desktop frontend trees (`browser/`, `toolkit/`, `devtools/`) do not cover: `widget/nsIMacSharingService.idl` with `widget/cocoa/nsMacSharingService.mm` (Objective-C++ — the macOS share sheet, `getSharingProviders`, `openSharingPreferences`), and `widget/nsIWindowsUIUtils.idl`'s `shareUrl` for Windows. So "the Share menu is empty", "the wrong apps are listed", and "Share does nothing" are usually localized in `widget/`, per-OS, and are **not** out of scope for being C++ rather than JS. Note which OS the bug is about before reading either. **Two unrelated things are called "sharing" in this tree.** This component is sharing a URL _out_ to another app. Screen, camera and microphone sharing — the sharing indicator, "stop sharing" button, and per-tab sharing state — is WebRTC, lives in `browser/actors/WebRTCParent.sys.mjs`, and belongs to site permissions. A grep for `sharing` returns both, so check which one the report is actually about; a bug about an indicator or a "stop sharing" control is almost certainly the WebRTC one. ## Tests -browser-chrome under `browser/components/contentsharing/tests/browser/`, which has a `ContentSharingMockServer.sys.mjs` for the remote config — use it rather than stubbing the fetch yourself. Schema fixtures are xpcshell under `tests/unit/` (`validContentSharing.*.json` / `invalidContentSharing.*.json`), so a config-parsing bug has a very cheap regression test. The `widget/` half is effectively uncovered: there is no automated test for the macOS share sheet or the Windows share dialog, so for a platform-side bug say the area is untested rather than leaving the reader wondering. +browser-chrome under `browser/components/sharing/tests/browser/`, which has a `ContentSharingMockServer.sys.mjs` for the remote config — use it rather than stubbing the fetch yourself. Schema fixtures are xpcshell under `browser/components/sharing/tests/unit/` (`validContentSharing.*.json` / `invalidContentSharing.*.json`), so a config-parsing bug has a very cheap regression test. The `widget/` half is effectively uncovered: there is no automated test for the macOS share sheet or the Windows share dialog, so for a platform-side bug say the area is untested rather than leaving the reader wondering.