Route four more components for frontend triage, and stop writing the component list in four places - #6617
Open
msujaws wants to merge 5 commits into
Open
Conversation
…d and where they report
`d3f0ed05` added `Firefox :: Site Permissions` across six files, and four of those
edits were the same component name written into three differently worded prose lists
plus a test. Nothing kept the four in step, and the one that matters is invisible when
it goes wrong: `channel_for` fails closed, so a component missing from `SLACK_CHANNELS`
still gets its comment and severity change applied unattended, with nobody told.
`TRIAGE_SCOPE` in config.py now holds each routed component once, as a
`ScopedComponent(product, component, area, channel)`. `SLACK_CHANNELS` is derived from
it, so notify.py is unchanged -- it keeps its `.strip()`, its no-default `.get()`, and
one flat mapping to look up. `render_scope` in agent.py renders the same tuple into a new
`Components in scope` section of system.md, which is a `str.format` template already.
`channel` is required rather than optional on purpose. An entry without one would be a
component under unattended triage with nobody told, which is what `channel_for` failing
closed already produces by accident; there is no reason to be able to express it
deliberately. So the registry is exactly the routed set, and it should stay in step with
bugbot's `TRIAGED_COMPONENTS`, which decides what arrives automatically. The two are
identical today.
The rendered section is careful about what the list is not, because a list of components
in a system prompt invites two expensive misreadings. Read as exhaustive, it declares an
in-scope bug out of scope -- the `ecea6ca6` mistake -- so the section says outright that
it is not the limit of what gets triaged and names `scoping.md` as what decides scope:
any user-facing Firefox defect qualifies, listed or not. Read as a vocabulary, it gets a
bug's component adjusted to match, and since the component is also the routing key,
notify.py then tells nobody; so it also asks for `product` and `component` verbatim from
Bugzilla.
Only the enumeration is generated. The per-area guidance under `Source repository` stays
hand-authored markdown: the Site Permissions bullet is six clauses of hedging, and moving
it into an implicitly-concatenated literal inside a NamedTuple -- reflowed by
ruff-format, where `{` needs no doubling but the surrounding system.md does -- would make
the authoring worse to save an edit that only happens when a component's code layout is
genuinely new. The bullets are also keyed by area, not component.
Not named `TRIAGED_COMPONENTS`, even though it currently holds the same pairs as bugbot's
tuple of that name. They answer different questions -- bugbot's is what to send, this is
where to report -- and they are in separate repos with separate deploys, so they can
legitimately differ for a release. One name for both would hide that.
Four new tests, and 46 pass. `test_the_channel_belongs_to_the_component` now loops the
registry instead of asserting a line per component, so a new component needs no test
edit; the invariants a loop cannot express -- whitespace stripping, fail-closed on an
unlisted or garbled pair, `Firefox :: History` being nothing at all, two components
sharing a channel -- stay written out. `test_the_registry_names_each_component_once`
covers the duplicate key that would collapse silently in the derived dict, and
`test_every_channel_is_a_channel_name` the missing `#` that becomes `channel_not_found`
at apply time, after the Bugzilla writes have landed.
`test_the_scope_says_it_is_neither_a_limit_nor_a_vocabulary` covers both misreadings
above. `test_every_area_has_prompt_guidance` is the load-bearing one: it fails when an
area has no `Source repository` bullet, which is how a component gets triaged with the
agent having no idea where its code lives.
… now that the registry renders it The same set was enumerated in three places, in three wordings, with nothing keeping them in step: `rules/frontend-triage.md` named nine desktop components, `rules/scoping.md` named seven overlapping ones, and README.md named eight plus repeated the whole routing table from config.py. `d3f0ed05` had to edit all three to add one component, and the README table is the one that had already drifted into telling the reader to open config.py while also copying it. All three now point at **Components in scope**, which f076cc9e renders from `TRIAGE_SCOPE`. The prose that was doing work stays: scoping.md keeps the install-and-update paragraph, frontend-triage.md keeps everything below its opening sentence, and the README keeps the four area descriptions with the language each implies, since those change on the order of once a year rather than per component. This does change what the model sees, and in the direction we want. The list moves out of `rules/*.md`, which the agent globs and reads only when it judges a ruleset relevant, and into system.md, which is unconditionally in context. frontend-triage.md already deferred upward for the per-area layout, so this follows the existing grain -- deferring the other way, with the system prompt pointing at an optionally-read file for the scope, would not be safe. The deleted lists were wider than `TRIAGE_SCOPE` -- they named Address Bar, Menus, Theme, Session Restore and others that route nowhere -- so the rulesets now say explicitly what those lists only implied by ending in an ellipsis: `scoping.md`'s rule is that any user-facing Firefox defect is in scope, and a component's absence from the prompt's list is not a reason to skip a bug. Without that sentence this commit would have narrowed the agent's effective scope while appearing only to move a list. The README table is replaced by the four things about routing that are not obvious from reading the registry, plus one worked example: that the key is the component rather than the team, that `TRIAGE_SCOPE` is narrower than what the agent will triage and should stay in step with bugbot's `TRIAGED_COMPONENTS`, that there is no default channel, and that a garbled `product`/`component` matches nothing. The cost is that a reader who wants to know where one component routes now opens config.py. I considered keeping the table behind a test that regexes its rows back out and compares them to `SLACK_CHANNELS`, and decided a six-row list did not earn the machinery -- and prettier realigns every row when the widest cell changes, so "add one row" is a six-row diff either way. 46 tests pass unchanged; none of them read the markdown.
…and tell the agent there are two toolbars Two `ScopedComponent` entries, both `#android-core-dev`, which already receives `Firefox for Android :: History` -- so this needs no Slack work and can ship without waiting on a channel. That is the whole functional change; f076cc9e made the routing tests derive from `TRIAGE_SCOPE`, so there is no test edit and no prose list to update. The prompt is where the work is, because both components sit on a localization trap of the same kind as the stub-versus-full installer the prompt already warns about. There are **two** toolbars: the browser one at `…/fenix/components/toolbar/` and the homepage's own at `…/fenix/home/toolbar/`. Nothing in either name says which surface it serves, so a `Homepage` bug can be localized into a toolbar file and a `Toolbar` bug into the homepage. Under both, android-components carries two generations of the widget -- the Compose `components/compose/browser-toolbar/` and the View-based `components/browser/toolbar/`, with the interface in `concept/toolbar/` and the session wiring in `feature/toolbar/`. A fix planned against the retired implementation reads correct and changes nothing, so the bullet says to confirm which one Fenix builds. Same reason the Fenix-wide Compose migration is now called out on the parent bullet: a screen can have both a `…View.kt` and a `…Composable.kt` with only one live. The homepage is one screen assembled from twelve section subpackages (`topsites/`, `pocket/`, `recenttabs/`, …), so "which section" comes before "which file" and a top-sites bug is not in `Homepage.kt`. `Firefox for Android` also has separate components for several of those sections -- `Top Sites`, `Stories`, `Collections`, `Bookmarks`, `Menu`, `Search` -- so one package is reachable from more than one component, and `Stories` is `home/pocket/` in the tree because the rename never happened. The bullet says to triage the bug under the component it was filed in, which matters twice over: the component also routes the notification. The test-discovery bullet gained the mirror rule, since `HomeFragmentTest.kt` is the easiest thing to find and almost never the right answer, and a note that a Compose surface may only be covered by an `androidTest` UI test -- worth saying rather than reporting no coverage. Volume is modest and lopsided: over the last 90 days, 35 open defects in Homepage of which 24 are staff-filed, against 18 and 2 for Toolbar, so Homepage will be most of this. New Tab Page is 148 and 90 for comparison. `max_triggers` stays at 3. 46 tests pass. bugbot must not start sending these until this is deployed -- without a `SLACK_CHANNELS` entry, `channel_for` fails closed and the comment and severity change land with nobody told.
… tell the agent which of its two state machines it is looking at 22 of the 32 open defects filed in `Firefox :: IP Protection` over the last 90 days came from staff, second only to New Tab Page's 90, so this is the highest-volume component added in a while. `TRIAGE_SCOPE` gets one entry. Both of its directories already sit inside the `browser/` and `toolkit/` list the prompt gives, so unlike Site Permissions this needs no rescue from being read as a Core bug -- what it needs is to know that the module is split and that the symptom and the cause are usually on opposite sides. The panel is in `browser/components/ipprotection/`; the state is in `toolkit/components/ipprotection/`. There are **two** state machines, both with a `READY`, and conflating them is the mistake worth pre-empting: `IPProtectionStates` in `IPProtectionService.sys.mjs` is entitlement and sign-in (`UNINITIALIZED`, `UNAVAILABLE`, `UNAUTHENTICATED`, `READY`, firing `IPProtectionService:StateChanged`), while `IPPProxyStates` in `IPPProxyManager.sys.mjs` is the connection (`NOT_READY`, `READY`, `ACTIVATING`, `ACTIVE`, `ERROR`, `PAUSED`, firing `IPPProxyManager:StateChanged`). "It showed connected when it was not" is the second and lives in `toolkit/`, even though every visible trace of it is in the panel -- so an agent that starts where the screenshot points lands in the wrong tree. The bullet also sends the agent to `toolkit/components/ipprotection/docs/`, which has `StateMachine.rst`, `Preferences.rst`, `Constants.rst` and `Components.rst`. No other area here has in-tree prose docs, and reading them beats reconstructing the machine from source. Two corrections it needs on top of the generic advice: prefs are `browser.ipProtection.*` in `browser/app/profile/firefox.js`, not `modules/libpref/init/all.js` where the prompt's generic pref line points (25 hits against 0), and a `browser/` to `toolkit/` split is still in flight, so Searchfox is more trustworthy than the shallow checkout for a path. `severity-assessment.md` gets a paragraph, in the same shape as the install-and-update one. Turning the VPN off is not a workaround for the VPN not working; it is the absence of the thing the user is paying for. So start from S2 rather than the S3 a papercut gets. The paragraph then asks for a distinction the reports will not make on their own: state merely *displayed* wrong is a UI bug, state actually wrong means traffic is unproxied and belongs above S2. Both arrive as "it said I was protected". The README's area list now matches the registry's six areas. It had four, having grouped the updater and the installer as "install and update" -- which reads fine as prose but left a reader unable to tell that IP Protection is triaged at all, and the list is now the only place the README names what is in scope. `TRIAGE_TASK` in `__main__.py` already reads "user-facing Firefox bug" after ecea6ca, so it needs no widening; the `description` in services/hackbot-api is console UI text that never reaches the model, and IP Protection is desktop frontend for its purposes. Neither is touched. 46 tests pass, with no test edit for the routing -- f076cc9e derives it. The new `IP Protection` area is what `test_every_area_has_prompt_guidance` was for: it failed until the prompt bullet existed. **This must be deployed before bugbot starts sending**, and before that someone has to confirm `#team-eng-ip-protection-triage` is the exact name and is public. `channel_for` fails closed, so a wrong name means the comment and the severity change still land with nobody told, and a private channel the app was not invited to fails the same way -- `chat:write.public` only covers public ones.
… agent the platform half lives in widget/ 34 of the 38 open defects filed in `Firefox :: Sharing` over the last 90 days came from staff, more than IP Protection's 22, so this is a high-volume addition. The prompt bullet is the point of this commit, because `Sharing` is the one area here whose code reaches outside the `browser/`, `toolkit/`, `devtools/` list the desktop frontend bullet gives. `browser/modules/SharingUtils.sys.mjs` populates the menu and gates on `BrowserUtils.getShareableURL`, and `browser/components/contentsharing/` holds the newer remotely-configured piece, validated against `contentsharing.schema.json`. But the platform half is `widget/nsIMacSharingService.idl` with `widget/cocoa/nsMacSharingService.mm`, and `shareUrl` on `widget/nsIWindowsUIUtils.idl`. So "the Share menu is empty", "the wrong apps are listed" and "Share does nothing" are per-OS bugs localized in Objective-C++ or C++ that the prompt would otherwise imply are out of scope -- the same trap `d3f0ed05` had to fix for the C++ permission store. The bullet also separates the two unrelated things this tree calls sharing. This component is sharing a URL out to another app; the sharing indicator, the "stop sharing" button and per-tab sharing state are WebRTC screen and camera capture in `browser/actors/WebRTCParent.sys.mjs`, which is site permissions' area. A grep for `sharing` returns both, and the WebRTC one has far more hits, so an agent that greps first lands in the wrong component. The test bullet names the `ContentSharingMockServer.sys.mjs` helper rather than leaving the agent to propose stubbing the config fetch by hand, points at the valid/invalid schema fixtures under `tests/unit/` as the cheap regression anchor for a config-parsing bug, and says outright that the `widget/` half has no automated coverage -- better than an empty `relevant_tests` the reader cannot interpret. No severity paragraph. Sharing failing has an in-product workaround: copy the link. The S3 papercut default is right, unlike the updater or the VPN. Two files, which is what a component in a new area costs now: the registry entry plus the `Source repository` bullet that `test_every_area_has_prompt_guidance` demands. 46 tests pass. Deploy before the matching bugbot change, and confirm `#content-sharing-automation` exists and is public first -- `channel_for` fails closed, so a wrong name still lets the comment and severity change land with nobody told.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four components join the frontend-triage agent's routing:
Firefox for Android :: ToolbarandFirefox for Android :: Homepageto#android-core-dev,Firefox :: IP Protectionto#team-eng-ip-protection-triage, andFirefox :: Sharingto#content-sharing-automation. All four channels exist and are public.Adding one component used to touch six files, and #6610 (Site Permissions) is the worked example: one line of routing in
config.py, then the same component name written intorules/frontend-triage.md,rules/scoping.md, two places inREADME.md, and one assertion intests/test_notify.py. Nothing kept those in step, and the failure is silent in the direction that matters —channel_forfails closed, so a component missing fromSLACK_CHANNELSstill gets its comment and severity change applied unattended, with nobody told. Rather than pay that four more times, the first two commits collapse it.The registry
TRIAGE_SCOPEinconfig.pynow holds each routed component once, as aScopedComponent(product, component, area, channel).SLACK_CHANNELSis derived from it, sonotify.pyhas zero diff — it keeps its.strip(), its no-default.get(), and one flat mapping to look up.render_scopeinagent.pyrenders the same tuple into a newComponents in scopesection ofsystem.md, which was already astr.formattemplate.channelis required rather than optional. An entry without one would be a component under unattended triage with nobody told, which is what failing closed already produces by accident; there is no reason to be able to express it on purpose. So the registry is exactly the routed set, and it is exactly bugbot'sTRIAGED_COMPONENTStoday.Only the enumeration is generated. The per-area guidance under
Source repositorystays hand-authored markdown — that prose is the payload, not the tax. The Site Permissions bullet is six clauses of careful hedging; moving it into an implicitly-concatenated literal inside a NamedTuple, reflowed byruff-format, where{needs no doubling but the surroundingsystem.mddoes, would make authoring worse to save an edit that only happens when a component's code layout is genuinely new.What the components needed told
Three of the four are not just a routing line, and that is most of this diff.
Firefox :: Sharingis the one area whose code reaches outsidebrowser/,toolkit/,devtools/. The frontend isbrowser/modules/SharingUtils.sys.mjsandbrowser/components/contentsharing/, but the share sheet itself iswidget/cocoa/nsMacSharingService.mmandshareUrlonwidget/nsIWindowsUIUtils.idl. So "Share does nothing on macOS" is a per-OS bug in Objective-C++ that the prompt would otherwise imply is out of scope — the same trap #6610 fixed for the C++ permission store. The bullet also separates the two unrelated things this tree calls sharing: WebRTC screen/camera capture dominates a grep forsharingand belongs to site permissions.Firefox :: IP Protectionhas two state machines that both have aREADY, and conflating them is the mistake worth pre-empting.IPProtectionStatesinIPProtectionService.sys.mjsis entitlement and sign-in;IPPProxyStatesinIPPProxyManager.sys.mjsis the connection. "It showed connected when it was not" is the second and lives intoolkit/, even though every visible trace of it is in the panel — so an agent that starts where the screenshot points lands in the wrong tree. The bullet also sends it totoolkit/components/ipprotection/docs/, which is real in-tree prose documentation no other area here has, and corrects the prefs path (browser/app/profile/firefox.js, notmodules/libpref/init/all.js: 25 hits against 0).Firefox for Android :: Toolbarand:: Homepageare in the existing Android area, so they needed no new bullet — but both carry a stub-versus-full-installer style trap. There are two toolbars (fenix/components/toolbar/and the homepage's ownfenix/home/toolbar/) over two generations of the android-components widget, so a fix planned against the retired one reads correct and changes nothing. And the homepage is one package per section, so a top-sites bug is not inHomepage.kt— with sibling components (Top Sites,Stories,Collections) able to receive the same bug, andStoriesbeinghome/pocket/in the tree because the rename never happened.severity-assessment.mdgains one paragraph, for IP Protection only: turning the VPN off is not a workaround for the VPN not working, so start from S2 rather than the S3 a papercut gets. It also asks for a distinction the reports will not make on their own — state merely displayed wrong is a UI bug, state actually wrong means traffic is unproxied and belongs above S2. Android Toolbar and Homepage bugs are ordinary papercuts and keep the S3 default. Sharing has an in-product workaround (copy the link), so it does too.Volume
Open defects filed in the last 90 days, and the subset whose reporter contains
@mozilla.com— the same approximation that reproduced the "90 for New Tab Page" figure in #6610:Firefox :: SharingFirefox for Android :: HomepageFirefox :: IP ProtectionFirefox for Android :: ToolbarFor comparison, New Tab Page is 148/90 and Site Permissions is 2.
Testing
uv run --package hackbot-agent-frontend-triage pytest agents/frontend-triage/tests— 46 pass, and they pass at every commit in the series, not just at the tip.Four new tests, all in service of making the remaining per-component cost visible rather than forgettable:
test_every_area_has_prompt_guidance(tests/test_plan.py) is the load-bearing one. It fails when aTRIAGE_SCOPEarea has no bolded bullet underSource repository— which is exactly how a component gets routed with the agent having no idea where its code lives. I verified it fails by adding a component with areaPDF viewerand no bullet:AssertionError: PDF viewer.test_the_registry_names_each_component_oncecovers the duplicate key that would collapse silently in the derived dict — the failure mode a growing registry actually has, and one that is invisible in a diff that only adds a line.test_every_channel_is_a_channel_namecovers the missing#or stray capital that becomeschannel_not_foundat apply time, after the Bugzilla writes have already landed.test_the_scope_says_it_is_neither_a_limit_nor_a_vocabularycovers both ways a component list in a system prompt gets misread (below).test_the_channel_belongs_to_the_componentnow loopsTRIAGE_SCOPEinstead of asserting a line per component, so a new component needs no test edit. The invariants a loop cannot express stay written out: whitespace stripping, fail-closed on an unlisted or garbled pair,Firefox :: Historybeing nothing at all (it is not a real BMO component — the desktop one isBookmarks & History), and two components sharing a channel, each asserted separately.I also rendered the system prompt before and after the registry commit at an unchanged component set and diffed it, to confirm the commit moves no prose and rewords nothing.
Unrelated and pre-existing:
services/hackbot-api/testshas 3 failures and 8 errors on clean master, as ecea6ca's message records. This PR does not touch that service.Tradeoffs worth a reviewer's attention
The README routing table is gone. The section above it already said "Routing is
SLACK_CHANNELSinconfig.py", so the table told the reader to open a file and then copied it. It is replaced by the four things about routing that are not obvious from reading the registry, plus one worked example. The cost is real: to see where one component routes you now openconfig.py. I considered keeping the table behind a test that regexes its rows back out and compares them to the registry, and decided a six-row list did not earn the machinery — and prettier realigns every row when the widest cell changes, so "add one row" is a six-row diff either way. Happy to put it back if it is load-bearing for someone.The component list moved into the always-present system prompt, out of
rules/*.md, which the agent globs and reads only when it judges a ruleset relevant. That is a deliberate strengthening, andfrontend-triage.mdalready deferred upward for the per-area layout — but it does mean the rendered model context is not byte-identical before and after, and it is worth knowing that.A list of components in a system prompt invites two expensive misreadings, and the generated section says so outright. Read as exhaustive it declares an in-scope bug out of scope, which is the ecea6ca mistake;
scoping.md's rule has always been that any user-facing Firefox defect is in scope, and its list ended in an ellipsis. Read as a vocabulary it gets a bug's component adjusted to match, and since the component is also the routing key,notify.pythen tells nobody.This does not make every future component a one-liner, and I want to be straight about that. A component in an existing area is now genuinely one file and one line — I verified it by adding
Firefox :: Downloads PanelunderDesktop frontend: 1 file, 46 tests still pass, routing and the rendered prompt both pick it up. But of the nine components now in scope, six needed a new area, and the two Android ones needed prose anyway. The refactor removed the toil — five duplicated bookkeeping edits that carried no information and could silently drift — not the work, which is deciding whether the agent needs to be told where this component's code lives. What it did buy, as a side effect rather than a goal, is that the expensive part is now impossible to skip by accident.Landing order
This needs to be deployed before the matching bugbot change (mozilla/bugbot#2996, which adds the same four pairs to
TRIAGED_COMPONENTS). Without an entry here,channel_forfails closed: the notification is silenced but the run is not, so the analysis and the severity change still reach the bug with nobody told. bugbot's cron checks outreleases/latestrather than master, so that PR is inert until a release is cut — the gate is "this rolled out before that release", not merge order.Nothing here is keyed by component outside
agents/frontend-triage/. I checkedTRIAGE_TASKin__main__.pyand thefrontend-triagedescription inservices/hackbot-api/app/agents.py: both already read "user-facing Firefox" after ecea6ca, which covers all four, so neither is touched.