diff --git a/agents/frontend-triage/README.md b/agents/frontend-triage/README.md index d33fa01d1e..fe816b1644 100644 --- a/agents/frontend-triage/README.md +++ b/agents/frontend-triage/README.md @@ -14,14 +14,26 @@ human (or a downstream execution agent) takes it from there. ## What it triages **Defects in user-facing Firefox** — the kind documented with a screenshot, steps -to reproduce, or a log rather than a stack trace: - -- **Desktop frontend**, under `Firefox`: Tabbed Browser (incl. Split View and Tab - Groups), New Tab Page, Address Bar, Menus, Toolbars and Customization, Sidebar, - Site Permissions, Theme. -- **Firefox for Android**: History. Kotlin under `mobile/android/fenix/`. -- **Install and update**: `Firefox :: Installer` (NSIS) and - `Toolkit :: Application Update` (`.sys.mjs`, IDL, C++). +to reproduce, or a log rather than a stack trace. `scoping.md` is what decides +scope, and it is broad: any user-facing Firefox defect qualifies. + +What is _routed_ is narrower. `TRIAGE_SCOPE` in `config.py` lists the components +bugs normally arrive from, one entry each, carrying the Slack channel and the area +whose code layout `prompts/system.md` describes. A bug handed to the agent by hand +in some other component — `Firefox :: Menus`, say — is triaged the same way and +reports to nobody. The areas, which are also how the rendered scope list is +grouped: + +- **Desktop frontend**, under `Firefox`. JS/JSM modules, CSS, XUL/HTML. +- **Site permissions**, also desktop, but split across the doorhanger, the state, + and a C++ store outside `browser/`. +- **IP Protection**, the built-in VPN. Panel UI in + `browser/components/ipprotection/`, the proxy and entitlement state machines in + `toolkit/components/ipprotection/`. +- **Firefox for Android**. Kotlin under `mobile/android/fenix/` and + `mobile/android/android-components/`. +- **Application updater** — `Toolkit :: Application Update` (`.sys.mjs`, IDL, C++). +- **Windows installer** — `Firefox :: Installer` (NSIS). Install and update bugs are the odd ones out: they arrive as a failure with an error code and an `update.log` or installer log, usually with no steps to @@ -31,6 +43,11 @@ framing reads as a reason to skip them. `severity-assessment.md` starts them at S2 rather than the S3 a papercut would get, since a user who cannot update is left on an unpatched build with no in-product workaround. +IP Protection has the same S2 floor, for the same reason: turning the VPN off is +not a workaround for it not working. It carries one extra instruction, because the +distinction does not survive a bug report — state merely _displayed_ wrong is a UI +bug, while state actually wrong means traffic is unproxied and belongs above S2. + Poor fits: crashes, hangs, assertions and sanitizer reports (those belong to [`bug-fix`](../bug-fix/)) — note that "the installer failed" is not a crash report — anything outside user-facing Firefox, and bugs whose fix can only be @@ -173,22 +190,27 @@ The audience is the team whose bug was just written to by nobody, so only an auto-applied run notifies. A medium or low result wrote nothing to Bugzilla and stays silent, even if someone applies it by hand later. -Routing is `SLACK_CHANNELS` in `config.py`, keyed by `" :: "`: - -| Product :: Component | Channel | -| -------------------------------- | ------------------------------- | -| `Firefox :: New Tab Page` | `#hnt-dev-triage` | -| `Firefox for Android :: History` | `#android-core-dev` | -| `Toolkit :: Application Update` | `#installer-updater-bug-triage` | -| `Firefox :: Installer` | `#installer-updater-bug-triage` | -| `Firefox :: Site Permissions` | `#privacy-team-automation` | - -Two components may share a channel, as the installer and the updater do; the key is -the component, not the team. A component that is not listed notifies nobody; there is -deliberately no default channel, since posting one team's triage into another team's -channel is worse than silence. Product and component come from the agent's -`product`/`component` plan fields, because nothing else carries them out of a run -whose only input is a bug id, so a garbled value matches no team and sends nothing. +Routing is the `channel` on each `TRIAGE_SCOPE` entry in `config.py`, looked up by +`" :: "` through the derived `SLACK_CHANNELS` — so +`ScopedComponent("Firefox", "New Tab Page", "Desktop frontend", "#hnt-dev-triage")` +sends a New Tab Page run to `#hnt-dev-triage`. + +Four things about that which are not obvious from reading the registry: + +- **The key is the component, not the team**, so two components may share a channel, as + the installer and the updater do, without either knowing about the other. +- **There is deliberately no default channel**, since posting one team's triage into + another team's channel is worse than silence. +- **`TRIAGE_SCOPE` is narrower than what the agent will triage.** It is the routing + table, and it should stay in step with bugbot's `TRIAGED_COMPONENTS`, which decides + what arrives automatically. `scoping.md` puts _any_ user-facing Firefox defect in + scope, so a bug handed to the agent by hand in some other component is triaged + normally and reports to nobody. The system prompt says so explicitly, because a list + of components read as exhaustive is how an in-scope bug gets declared out of scope. +- **Product and component come from the agent's `product`/`component` plan fields**, + because nothing else carries them out of a run whose only input is a bug id. A garbled + value matches no team and sends nothing, which is why the system prompt asks for them + verbatim even for components the scope list does not name. `notify.py` builds and records the message; the wording is code, not a model turn, so `slack.post_message` is _not_ in `ENABLED_ACTION_TYPES` and the agent is never @@ -205,12 +227,14 @@ notifies. The run page shows the failed action. `rules/` and `prompts/` both live under `hackbot_agents/frontend_triage/`. - **`rules/`** is the main behavior dial. `scoping.md` decides what gets skipped; - `frontend-triage.md` sets in-scope components, comment content, and the - confidence thresholds for recording an action. The agent globs the directory - and reads only what it judges relevant, so new `.md` files extend it — see - `rules/README.md` for how to author one. + `frontend-triage.md` sets comment content and the confidence thresholds for + recording an action. The agent globs the directory and reads only what it judges + relevant, so new `.md` files extend it — see `rules/README.md` for how to author + one. Neither file lists components; `TRIAGE_SCOPE` in `config.py` does. - **`prompts/system.md`** holds the standing instructions: output format, the - read-only mandate, and when to reach for Searchfox versus reading a file. + read-only mandate, when to reach for Searchfox versus reading a file, and the + per-area code layout. A component in a new area needs a **Source repository** + bullet here, and `tests/test_plan.py` fails until it has one. - **Cost** scales with tool use, not just turns — Searchfox results are token-heavy, so narrowing queries (`path_filter`, a modest `limit`) matters more than `MAX_TURNS` when batching. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py b/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py index 2b8ef00f70..4e76a41548 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py @@ -49,6 +49,8 @@ ENABLED_ACTION_TYPES, MOZILLA_VCS_TOOLS, SEARCHFOX_TOOLS, + TRIAGE_SCOPE, + ScopedComponent, ) from .hooks import add_comment_hook, update_bug_hook @@ -140,6 +142,44 @@ class FrontendTriageResult(HackbotAgentResult): ) +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. + + 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. + """ + by_area: dict[str, list[str]] = {} + for entry in scope: + by_area.setdefault(entry.area, []).append(entry.key) + + lines = [f"- **{area}** — {', '.join(keys)}." for area, keys in by_area.items()] + + return "\n".join( + lines + + [ + "", + # Two failure modes to close off, in order of how much they cost. Reading the + # list as exhaustive gets an in-scope bug declared out of scope, which is the + # `ecea6ca6` mistake. Reading it as a vocabulary gets a component "tidied" to + # match, and since the component is also the routing key, `notify.py` then + # silently tells nobody. + "**This list is not the limit of what you triage.** It is where bugs " + "normally come from, and which team each one reports to. `scoping.md` is " + "what decides scope: any user-facing Firefox defect is in scope, including " + "in a component not named above — triage it normally rather than calling it " + "out of scope for being absent here.", + "", + "It is also **not** a vocabulary for the `product` and `component` fields " + "of your plan. Copy those from Bugzilla verbatim, even when they are not " + "listed above.", + ] + ) + + def load_system_prompt(rules_dir: Path, extra: str) -> str: tmpl = (HERE / "prompts" / "system.md").read_text() @@ -147,6 +187,7 @@ def load_system_prompt(rules_dir: Path, extra: str) -> str: rules_dir=str(rules_dir.resolve()), extra_instructions=extra or "(none)", searchfox_links=SEARCHFOX_LINKS_PROMPT, + triaged_components=render_scope(), ) diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py index df4ac28749..c01bc06f05 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py @@ -1,3 +1,5 @@ +from typing import NamedTuple + # Bugzilla MCP tool names as exposed to the agent (mcp____). BUGZILLA_READ_TOOLS = [ "mcp__bugzilla__search_bugs", @@ -42,24 +44,86 @@ "bugzilla.update_bug", ] -# Where an auto-applied run reports itself, by `" :: "`. A channel -# belongs to the team that owns the component, so the routing does too: a component -# that is not listed sends nothing, since posting one team's triage into another team's -# channel is worse than silence. There is deliberately no default channel. + +class ScopedComponent(NamedTuple): + """A Bugzilla component sent here for triage, and where a finished run reports it.""" + + 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. + 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 + + @property + def key(self) -> str: + return f"{self.product} :: {self.component}" + + +# 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 +# one entry here rather than the same name written into three prose lists and a test. +# +# This is narrower than what the agent will triage. `rules/scoping.md` puts any +# user-facing Firefox defect in scope, and a bug handed to the agent by hand is triaged +# on that rule whether or not its component is named here -- it just reports to nobody. +# What this tuple decides is routing, and it should stay in step with bugbot's +# `TRIAGED_COMPONENTS`, which decides what arrives automatically. +# +# A channel belongs to the team that owns the component, so the routing does too: a +# component that is not listed sends nothing, since posting one team's triage into +# another team's channel is worse than silence. There is deliberately no default channel. # # `slack.post_message` is left out of `ENABLED_ACTION_TYPES` on purpose. The message is # code (see notify.py), not a model turn, so it goes through the recorder directly and # the agent is never given the tool — it has no say in what is said or where. -SLACK_CHANNELS = { - "Firefox :: New Tab Page": "#hnt-dev-triage", - "Firefox for Android :: History": "#android-core-dev", +# +# Ordered by area, grouped by first appearance — `render_scope` preserves that order, so +# this is also the order the model reads. There is no separate list of areas to keep in +# sync with this one. +TRIAGE_SCOPE = ( + ScopedComponent("Firefox", "New Tab Page", "Desktop frontend", "#hnt-dev-triage"), + ScopedComponent( + "Firefox", "Site Permissions", "Site permissions", "#privacy-team-automation" + ), + ScopedComponent("Firefox", "Sharing", "Sharing", "#content-sharing-automation"), + ScopedComponent( + "Firefox", + "IP Protection", + "IP Protection", + "#team-eng-ip-protection-triage", + ), + ScopedComponent( + "Firefox for Android", "History", "Firefox for Android", "#android-core-dev" + ), + ScopedComponent( + "Firefox for Android", "Toolbar", "Firefox for Android", "#android-core-dev" + ), + ScopedComponent( + "Firefox for Android", "Homepage", "Firefox for Android", "#android-core-dev" + ), # The installer and the updater are triaged by the same team, so two components # share a channel. Keying by product-and-component rather than by channel is what # lets them, without either one having to know about the other. - "Toolkit :: Application Update": "#installer-updater-bug-triage", - "Firefox :: Installer": "#installer-updater-bug-triage", - "Firefox :: Site Permissions": "#privacy-team-automation", -} + ScopedComponent( + "Toolkit", + "Application Update", + "Application updater", + "#installer-updater-bug-triage", + ), + ScopedComponent( + "Firefox", "Installer", "Windows installer", "#installer-updater-bug-triage" + ), +) + +# Where an auto-applied run reports itself, by `" :: "`. Derived, so +# that `notify.py` keeps one flat mapping to look up. +SLACK_CHANNELS = {c.key: c.channel for c in TRIAGE_SCOPE} # What a `bugzilla.update_bug` from this agent may touch. Enforced at record time # by `hooks.update_bug_hook`, so an out-of-bounds change is refused while the agent 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 23db2830d6..4971b288d4 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/prompts/system.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/prompts/system.md @@ -27,6 +27,10 @@ Do not claim to have "verified" or "tested" a fix. You are reasoning from the co Use **only** these tools for accessing Bugzilla, nothing else. +# Components in scope + +{triaged_components} + # Source repository 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. @@ -35,15 +39,26 @@ 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. -- **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`. +- **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. +- **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/`. +- 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. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md index a972512bbb..09d87a1b70 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md @@ -1,14 +1,11 @@ # User-facing Firefox defect triage These rules apply to **defects in user-facing Firefox** — the desktop frontend, -Firefox for Android, and the Windows installer and application updater. Typical -components: - -- Desktop frontend, all under `Firefox`: `Tabbed Browser`, - `Tabbed Browser: Split View`, `New Tab Page`, `Address Bar`, `Menus`, - `Toolbars and Customization`, `Sidebar`, `Site Permissions`, `Theme`. -- Android: `Firefox for Android :: History`. -- Install and update: `Firefox :: Installer`, `Toolkit :: Application Update`. +Firefox for Android, and the Windows installer and application updater. **Components in +scope** in the system prompt lists the components bugs normally arrive from, grouped by +the area whose code layout **Source repository** describes. Any user-facing Firefox +defect is in scope, though, whether or not its component is on that list; see +`scoping.md`. Desktop and Android bugs here are usually UI/UX papercuts, documented with a **video or screenshot** and steps to reproduce. diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/scoping.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/scoping.md index 8ea706488b..4832b08fc5 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/scoping.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/scoping.md @@ -28,11 +28,11 @@ output's `actionable` to `false`, `confidence` to `low`, and `root_cause` to nul ## Proceed normally Everything else — a `defect` in a user-facing Firefox component without the skip signals -above — is in scope. That covers the desktop frontend (Bookmarks & History, New Tab Page, -Session Restore, Sidebar, Site Permissions, Tabbed Browser: Split View / Tab Groups, -Toolbars and Customization, …), Firefox for Android (History, …), and install and update -(`Firefox :: Installer`, `Toolkit :: Application Update`). Continue to the -`frontend-triage` ruleset. +above — is in scope. That covers the desktop frontend, Firefox for Android, and install +and update. **Components in scope** in the system prompt lists the components bugs +normally arrive from, but it is not a limit: a user-facing Firefox defect in a component +that is not on it is still in scope, and being absent from it is not a reason to skip a +bug. Continue to the `frontend-triage` ruleset. An install or update failure reported as an error code and a log, with no steps to reproduce and no screenshot, is an in-scope defect — it is the usual shape of a bug in diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/severity-assessment.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/severity-assessment.md index 9ee14a3640..bf62f41956 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/severity-assessment.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/severity-assessment.md @@ -28,6 +28,16 @@ user is affected, how many users hit it, and whether a workaround exists. failure specific to one antivirus product or one locale is narrower than one that hits a whole channel or OS version — but start from **S2** and move up or down from there, rather than starting from S3. +- **IP Protection failures do not default to S3 either.** The user is paying for the + feature, and turning it off is not a workaround for it not working — it is the absence + of the thing they bought. A proxy that will not connect, that drops without saying so, + or that reports itself active while traffic goes around `IPPChannelFilter` leaves them + without the protection they believe they have. Start from **S2** and move up or down on + reach, rather than starting from S3. Separate two cases that read identically in a bug + report: the state is merely _displayed_ wrong (the panel disagrees with the proxy) or + it is actually wrong (traffic is unproxied). The first is a UI bug, the second is a + privacy exposure and belongs above S2. Say which one you concluded and what in the code + told you. - Weigh: is it functional vs cosmetic? Is there a workaround? How frequently and how broadly is it hit (mainline path vs rare configuration)? - Do **not downgrade** an existing higher severity unless you have strong evidence the diff --git a/agents/frontend-triage/tests/test_notify.py b/agents/frontend-triage/tests/test_notify.py index 318f1c653c..002601337d 100644 --- a/agents/frontend-triage/tests/test_notify.py +++ b/agents/frontend-triage/tests/test_notify.py @@ -10,6 +10,7 @@ FrontendTriageResult, SeverityAssessment, ) +from hackbot_agents.frontend_triage.config import TRIAGE_SCOPE from hackbot_agents.frontend_triage.notify import ( build_message, channel_for, @@ -87,13 +88,36 @@ def test_a_missing_summary_leaves_the_bug_link_alone(): def test_the_channel_belongs_to_the_component(): - assert channel_for("Firefox", "New Tab Page") == "#hnt-dev-triage" - assert channel_for("Firefox for Android", "History") == "#android-core-dev" - assert channel_for("Firefox", "Site Permissions") == "#privacy-team-automation" + # Derived from the registry rather than one assert per component, so adding a + # component is one entry in config.py and no test edit. The invariants a loop + # cannot express are asserted concretely in the tests below. + for entry in TRIAGE_SCOPE: + assert channel_for(entry.product, entry.component) == entry.channel + + +def test_whitespace_around_either_half_is_stripped(): # Surrounding whitespace is the agent's, not Bugzilla's. assert channel_for(" Firefox ", " New Tab Page ") == "#hnt-dev-triage" +def test_the_registry_names_each_component_once(): + # A duplicate key collapses silently in the derived `SLACK_CHANNELS`, so the second + # entry's channel would win with nothing to show it had. That is the failure mode a + # registry has as it grows, and it is invisible in a diff that only adds a line. + keys = [entry.key for entry in TRIAGE_SCOPE] + assert len(keys) == len(set(keys)) + + +def test_every_channel_is_a_channel_name(): + # Slack rejects an unknown channel at apply time, long after the Bugzilla comment + # and severity change have landed, and the run page is the only place it shows. A + # missing `#` or a stray capital is the whole cost of that, so catch it here. + for entry in TRIAGE_SCOPE: + assert entry.channel.startswith("#"), entry.key + assert entry.channel == entry.channel.strip().lower(), entry.key + assert " " not in entry.channel, entry.key + + def test_the_installer_and_the_updater_share_a_channel(): # One team triages both, and a key that is off by a character notifies nobody # rather than failing, so each one is asserted rather than assumed from the other. @@ -105,10 +129,16 @@ def test_the_installer_and_the_updater_share_a_channel(): def test_an_unowned_component_has_no_channel(): # Fails closed rather than falling back to a default: another team's channel is a # worse outcome than silence. + # + # `Address Bar` is a component this agent will triage if handed one -- scoping.md + # puts any user-facing Firefox defect in scope -- but it is not in `TRIAGE_SCOPE`, + # so no team is told. That is the case worth pinning: in scope and unrouted are + # different questions, and only the second one is this function's business. assert channel_for("Firefox", "Address Bar") is None assert channel_for("Core", "New Tab Page") is None # A component name is only owned within its own product: `History` routes to - # #android-core-dev under Firefox for Android and nowhere at all under Firefox. + # #android-core-dev under Firefox for Android and nowhere at all under Firefox -- + # which has no `History` component in the first place, only `Bookmarks & History`. assert channel_for("Firefox", "History") is None assert channel_for("Firefox", None) is None assert channel_for(None, "New Tab Page") is None diff --git a/agents/frontend-triage/tests/test_plan.py b/agents/frontend-triage/tests/test_plan.py index 3f5ddecbae..cf40baca99 100644 --- a/agents/frontend-triage/tests/test_plan.py +++ b/agents/frontend-triage/tests/test_plan.py @@ -11,7 +11,9 @@ may_apply_unattended, parse_confidence, parse_plan, + render_scope, ) +from hackbot_agents.frontend_triage.config import TRIAGE_SCOPE, ScopedComponent def _block(body: str) -> str: @@ -25,6 +27,49 @@ def test_the_system_prompt_renders(): prompt = load_system_prompt(Path("rules"), "") assert '{"add": ["…"]}' in prompt assert "{rules_dir}" not in prompt + assert "{triaged_components}" not in prompt + # The component list reaches the prompt as full routing keys, since a bare component + # name would not say which product it belongs to. + assert "Firefox :: New Tab Page" in prompt + assert "Toolkit :: Application Update" in prompt + + +def test_the_scope_is_grouped_by_area_in_registry_order(): + # Asserted against a fixed registry rather than the real one, so this keeps testing + # the grouping when TRIAGE_SCOPE changes. + scope = ( + ScopedComponent("Firefox", "New Tab Page", "Desktop", "#one"), + ScopedComponent("Toolkit", "Application Update", "Updater", "#two"), + ScopedComponent("Firefox", "Theme", "Desktop", "#one"), + ) + rendered = render_scope(scope) + assert rendered.startswith( + "- **Desktop** — Firefox :: New Tab Page, Firefox :: Theme.\n" + "- **Updater** — Toolkit :: Application Update.\n" + ) + + +def test_the_scope_says_it_is_neither_a_limit_nor_a_vocabulary(): + # Two ways to misread a list of components in a system prompt, both expensive. + # Reading it as exhaustive declares an in-scope bug out of scope, which is the + # mistake ecea6ca6 was fixing. Reading it as a vocabulary gets a component adjusted + # to match, and the component is the Slack routing key, so the notification then + # goes nowhere without failing. + rendered = render_scope() + assert "not the limit" in rendered + assert "verbatim" in rendered + + +def test_every_area_has_prompt_guidance(): + # 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 def test_confidence_is_normalized():