diff --git a/agents/README.md b/agents/README.md index b53491edae..fb3cae8ac4 100644 --- a/agents/README.md +++ b/agents/README.md @@ -63,7 +63,7 @@ the repo's root `docker-compose.yml`, so running it is three steps: ```dotenv # .env (repo root) — never commit this file ANTHROPIC_API_KEY=sk-ant-... - BUGZILLA_API_URL=https://bugzilla.mozilla.org + BUGZILLA_API_URL=https://bugzilla.mozilla.org/rest BUGZILLA_API_KEY=... ``` diff --git a/agents/frontend-triage/README.md b/agents/frontend-triage/README.md index c7d092aa84..6208adb681 100644 --- a/agents/frontend-triage/README.md +++ b/agents/frontend-triage/README.md @@ -76,7 +76,7 @@ the secrets in a gitignored `.env` at the repo root: ```dotenv ANTHROPIC_API_KEY=sk-ant-... -BUGZILLA_API_URL=https://bugzilla.mozilla.org +BUGZILLA_API_URL=https://bugzilla.mozilla.org/rest BUGZILLA_API_KEY=... ``` @@ -109,7 +109,7 @@ they come from `.env`, `compose.yml`, or the command line. | `BUG_ID` | yes | The Bugzilla bug to triage | | `BROKER_URL` | yes | Bugzilla broker base URL; the agent appends `/mcp`. `compose.yml` sets it | | `ANTHROPIC_API_KEY` | yes | Drives the agent (billed per token) | -| `BUGZILLA_API_URL` | yes | e.g. `https://bugzilla.mozilla.org` — **broker container only** | +| `BUGZILLA_API_URL` | yes | e.g. `https://bugzilla.mozilla.org/rest` — **broker container only** | | `BUGZILLA_API_KEY` | yes | **Broker container only**; reads only. The agent never sees it | | `MODEL` | no | Defaults to `claude-opus-5` (`DEFAULT_MODEL` in `__main__.py`); pinned so runs are reproducible and comparable | | `MAX_TURNS` | no | Hard cap on loop iterations — a runaway guard, cut off if hit | diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py b/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py index dd70a9a850..ecf8d7a6c2 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/agent.py @@ -147,14 +147,18 @@ class FrontendTriageResult(HackbotAgentResult): f"Markdown link built from the `{SEARCHFOX_PLACEHOLDER}` placeholder, which " "is expanded into a revision-pinned Searchfox URL when your comment is " "recorded:\n\n" - f" [{_EXAMPLE_PATH}]({SEARCHFOX_PLACEHOLDER}/{_EXAMPLE_PATH})\n\n" + f" [tabgroup.js]({SEARCHFOX_PLACEHOLDER}/{_EXAMPLE_PATH})\n\n" "- Write the placeholder **literally**. Do not put a revision, `tip` or " "`HEAD` in it, and do not write a searchfox.org URL yourself — you do not " "know which revision is being linked.\n" "- After it, give the repo-relative path, plus a line anchor when you know " "the line: `#1234`, or `#1234-1250` for a range. No `L` prefix.\n" - "- Use the path as the link text, without backticks — backticked text does " - "not render as a link.\n" + "- **Use the file name alone as the link text**, not the full path: the " + "path is already in the URL, and repeating it inline is most of what makes " + "these comments hard to read. Keep the full repo-relative path in the URL. " + "When two files you cite share a name, add just enough parent directory to " + "tell them apart (`Crossword/Crossword.jsx`). No backticks around the link " + "text — backticked text does not render as a link.\n" "- Leave the paths in the trailing ```json plan block as **bare paths** — " "that block is parsed by a downstream tool, and a link there would corrupt " "it.\n" 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 7824ce0029..d226d26a29 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/prompts/system.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/prompts/system.md @@ -34,7 +34,7 @@ Use **only** these tools for accessing Bugzilla, nothing else. # 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. +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 behavior, and any existing tests that cover the area. Where to look, and what you will find there, depends on the bug's component. Every area and the trees it covers: @@ -83,13 +83,13 @@ Use these to raise your confidence and precision — but you still cannot build # Delegating to the investigator subagent -You have one generic subagent type: `investigator`. It has the same read-only tools you do (source repo + bugzilla read tools). **You write its full instructions dynamically** each time you spawn it — there is no fixed investigator behaviour. +You have one generic subagent type: `investigator`. It has the same read-only tools you do (source repo + bugzilla read tools). **You write its full instructions dynamically** each time you spawn it — there is no fixed investigator behavior. Use it when: - An assessment requires deep source-code reading that would pollute your main context - You need a focused answer to a specific question ("where is the split-view group line drawn?") -- You want to parallelise independent investigations +- You want to parallelize independent investigations When you spawn an investigator via the Task tool, write a complete, self-contained prompt: what to look at, what question to answer, what format to return. The investigator has no memory of previous spawns. @@ -134,7 +134,7 @@ Before calling any action tool, state in your response: - **What** action you are recording and **why** (cite the specific rule) - **Your confidence**: high / medium / low -Record exactly one `bugzilla_add_comment` with your fix plan, ending with the severity block described below. That comment is the **only** thing you can write to a bug — you have no tool that changes a field, so a severity, keyword, status or resolution can only be _suggested_ in the comment for a human to apply. +Record exactly one `bugzilla_add_comment` with your fix plan, ending with the severity sentence described below. That comment is the **only** thing you can write to a bug — you have no tool that changes a field, so a severity, keyword, status or resolution can only be _suggested_ in the comment for a human to apply. The tool is deliberately narrow, and a call outside what it accepts is refused with the reason (fix it and retry — a refused call records nothing, so it costs you nothing but the turn): @@ -145,20 +145,17 @@ The `reasoning` parameter is required and stored alongside the recorded comment. # Severity in the comment -End your comment with this block, exactly this shape, after the fix plan and separated from it by a horizontal rule: +End your comment with the severity as its own final paragraph, on **one line**, exactly this shape: ``` ---- - -Suggested severity: S4 - +Suggested severity: S3. Widgets show stale but non-destructive state on a newly opened tab, which is the default configuration for anyone with the crossword enabled; reloading the tab works around it. ``` +- **One line.** No horizontal rule above it, no heading, and no line break between the level and the reasoning. A rule and a stacked label turn one sentence into a section, which is how a short comment starts looking like a report. It still begins its own line, so keep the blank line before it. +- The level is followed by a period and then the impact that decided it, in one or two sentences on the same line. - **Do not state the bug's current severity.** A reader has the field on screen, and a comment is permanent while the field is not — "currently S4" is wrong the moment somebody changes it. -- The reasoning goes on the line directly below the level, with no blank line and no `Rationale:` label. -- Keep the blank line _before_ the `---`. Without it the line above becomes a heading. - Do **not** put your confidence in the comment. It belongs in `severity_assessment` in the structured output. -- **Omit the whole block, horizontal rule included, when your severity confidence is low or you could not assess severity at all** — an out-of-scope or unlocalized bug included. A level you are unsure of still reads as a judgment someone may act on, and a stray rule with nothing under it renders as an empty section. +- **Omit the sentence entirely when your severity confidence is low or you could not assess severity at all** — an out-of-scope or unlocalized bug included. A level you are unsure of still reads as a judgment someone may act on. - Declare the severity **once**. Naming the level again in your analysis gives the reader two claims that can disagree, and a comment that does is refused. Always be **brief** and to the point. Developers have limited time. Do **not** record private comments — all developers on the bug need to see them, and a private one is refused. 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 index 34187f50db..b57c1fe58e 100644 --- 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 @@ -1,6 +1,6 @@ # 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. +`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 behavior 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 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 dcee83c455..da5446b5c7 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,54 +1,41 @@ # 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. **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`. - -Typical components: - -- Desktop frontend, all under `Firefox`: `Tabbed Browser`, - `Tabbed Browser: Split View`, `New Tab Page`, `Address Bar`, `Menus`, - `Toolbars and Customization`, `Sidebar`, `Theme`. -- Android: `Firefox for Android :: History`. -- Install and update: `Firefox :: Installer`, `Toolkit :: Application Update`. -- Messaging System: `Firefox :: Messaging System`. +These rules apply to **defects in user-facing Firefox**. **Components in scope** in the +system prompt lists the components bugs normally arrive from, grouped by the area whose +code layout **Source repository** describes. That list is not a limit: any user-facing +Firefox defect is in scope whether or not its component is on it. See `scoping.md`. Desktop and Android bugs here are usually UI/UX papercuts, documented with a **video or screenshot** and steps to reproduce. +Screenshots you can look at: download one with `download_attachment` and `Read` the +file. Screen recordings you cannot, because this agent's image has no ffmpeg to pull +frames out of them, so downloading a video buys nothing here. When the only evidence +is a recording, triage from the description, the steps to reproduce, and the code, +and say plainly in your comment that you did not view the recording. Do not imply you +did. + **Install and update bugs look different, and that is not a reason to skip them.** An installer or updater bug is normally a _failure_ rather than a papercut: an update that did not apply, an install that rolled back, a version that stayed where it was, -a wrong or unhelpful error dialog — reported with an error or status code, an +a wrong or unhelpful error dialog, reported with an error or status code, an `update.log` or installer log excerpt, and an OS and channel rather than a screenshot. Triage those normally: read the log the reporter pasted, map the status code to its definition in `toolkit/mozapps/update/common/` or to the NSIS `.nsh` that emits it, and localize from there. Missing steps to reproduce is the norm in this area -and is not by itself grounds to call a bug unactionable — say what you would need +and is not by itself grounds to call a bug unactionable. Say what you would need instead. If the bug is a crash, assertion failure, or sanitizer report, this ruleset does not -apply — note that and stop. "The installer failed" and "the update did not apply" are -**not** crash reports. Also stop if the bug is not in user-facing Firefox at all — a -Core, DevTools-internals, or build-system bug — and say which area it looks like. +apply. Note that and stop. "The installer failed" and "the update did not apply" are +**not** crash reports. Also stop if the bug is not in user-facing Firefox at all, a +Core, DevTools-internals, or build-system bug, and say which area it looks like. ## What to produce 1. **Localize the cause in the source.** Where to look and what language to expect - depend on the component — see **Source repository** in the system prompt for the - per-area layout. In short: desktop frontend under `browser/`, `toolkit/`, and - `devtools/` (JS/JSM, CSS, XUL/HTML); Android under `mobile/android/` (Kotlin, - Fragment/Store/Middleware/View); the updater under `toolkit/mozapps/update/` - (`.sys.mjs`, IDL, C++); the installer under `browser/installer/windows/nsis/` - (NSIS `.nsi`/`.nsh`); the Messaging System under `browser/components/asrouter/`, - `browser/components/aboutwelcome/`, `toolkit/components/messaging-system/` - (JS/JSM, CSS, XUL/HTML, JSON, JSON Schema). Find the module, the markup or layout, - and any relevant pref (often `modules/libpref/init/all.js`, or `app.update.*` - for the updater) that governs the behaviour. Use the `investigator` subagent for - deep searches. + depend on the component. **Source repository** in the system prompt carries the + per-area layout; use the `investigator` subagent for deep searches. 2. **Confirm the area is still live.** Check the referenced code/strings still exist and aren't already changed by a recent commit. If the bug looks already fixed (e.g. cannot reproduce on a newer version per comments, or the code path @@ -56,34 +43,95 @@ Core, DevTools-internals, or build-system bug — and say which area it looks li inventing a fix. 3. **Write a fix plan**: root cause, the specific files/functions/selectors to change, and the approach. Prefer a comprehensive fix at the right level over a - spot fix. -4. **Assess severity.** Apply the `severity-assessment` rules to judge the bug's - severity from its user impact. It goes in the severity block at the end of your - comment and in the structured output — never on the bug's `severity` field, - which you cannot set. + spot fix. The full plan goes in the structured output; the comment carries only + its conclusion. +4. **Assess severity** per the `severity-assessment` rules. ## Comment -When the `duplicate_hunter` named a candidate, the comment opens with -`**Possible duplicate:** ` on its own line, above everything else — -see **Checking for a duplicate** in the system prompt. Nothing when it found none. - -Then a single brief comment (a few sentences) with: the suspected root cause, -the target file(s), and the proposed approach. Cite concrete paths, each as an -inline Markdown link to its Searchfox permalink (with a line anchor where you -know the line) per the **Linking source files** section of the system prompt. Do -not restate the whole bug. Do not claim the fix is verified — you did not run it. - -Close with the severity block — a horizontal rule, `Suggested severity: `, -then the reasoning — unless your severity confidence is low or you could not assess -it, in which case leave the block out entirely. Exact shape is in **Severity in the -comment** in the system prompt. +**Four to five sentences in total.** That is the whole comment, counting the severity +rationale and counting each numbered fix step as a sentence. The footer the runtime +appends does not count. Treat this as a hard budget: an earlier version of this file +asked for "a few sentences" and every run produced twelve to seventeen. + +The budget is affordable because the comment is not the handoff. `root_cause`, +`proposed_fix`, `target_files` and `relevant_tests` in the structured output carry the +detail for the executor and have no length limit. The comment exists so a human +scanning the bug learns, in one screenful, what is broken and what to do about it. Put +the depth in the JSON and keep the comment to its conclusion. + +A comment that fits is roughly one or two sentences naming the file and the mechanism, +one or two on the fix, and the closing severity sentence. Anything else has to earn its +sentence by displacing one of those. + +### Worked example + +From a different bug, in another area, to show the shape. The paths and line numbers +below are that bug's, not yours. Do not reuse them. + +``` +[nsUpdateService.sys.mjs]({{searchfox.permalink}}/toolkit/mozapps/update/nsUpdateService.sys.mjs#3410-3428) +gives up on the update when a partial patch fails verification, because +`#handlePatchFailure` is reached from the apply path and not from the download path, +so the complete-patch fallback at +[#3502]({{searchfox.permalink}}/toolkit/mozapps/update/nsUpdateService.sys.mjs#3502-3515) +never runs and the update stays pending across restarts. Route the download failure +through the same handler, and extend +[test_0113_general.js]({{searchfox.permalink}}/toolkit/mozapps/update/tests/unit_service_updater/test_0113_general.js) +with a partial-patch verification failure asserting the complete patch is fetched. + +Suggested severity: S2. A user on the release channel stops receiving updates entirely +after one corrupt partial, with no in-product workaround and nothing telling them +anything is wrong. +``` + +Three sentences and a severity sentence. It names two files, a private method, two line +anchors and a test, and it never narrates the investigation that found them. The +severity closes the comment on its own line, with no rule above it and no break between +the level and the reason. + +### How the comment reads + +Engineers have told us these comments are hard to follow: correct, but written like +an essay about an investigation rather than a note from one engineer to another. +"Be brief" did not fix it, so here are the specific habits to drop. Each one is +checkable against your draft before you record it. + +- **Open on the mechanism, not on a frame.** The first sentence names the file or + function and says what it does wrong. Cut openers that tell the reader how to + read what follows: "Two different mechanisms are in play here, and only one of + them is actually broken", "Unlike every other widget", "The interesting case is". +- **Do not rule out the alternatives you considered.** "Pref-backed state is not at + risk", "Redux widgets are unaffected", "the preloading machinery is fine" answer + questions the reader did not ask, and they are the single largest source of length + in these comments. What you eliminated goes in `reasoning`, not in the comment. +- **No em dashes.** Every one is a sentence that has not decided where it ends. Use + a period, a comma, or a colon. +- **No one-line paragraph used as a beat.** "The crossword does not." on its own + line is rhetoric. Fold it into the sentence carrying the evidence. +- **A parenthetical longer than about five words is a sentence.** Promote it or cut + it. Do not nest one inside another. +- **Recommend once.** A "worth auditing the other widgets too" or "worth confirming + with the vendor" bolted onto the end of a fix plan is a second, unranked proposal + competing with the first. Either it is a numbered step or it is not in the + comment. +- **Report the finding, not the search.** What you read and which tools you reached + for belong in `reasoning`. +- **Say the thing once.** If the root cause is in the first sentence, the fix does + not restate it and the severity rationale does not restate it again. +- **American English.** "behavior", "initialize", "generalize", "license", never + "behaviour", "initialise", "generalise", "licence". + +None of this is license to drop substance. Every file name, function name, line +anchor and Searchfox link that carried weight in the long version carries it in the +short one. The target is the same findings in fewer, flatter sentences, not fewer +findings. ## Confidence -Your `confidence` decides whether your comment reaches the bug unreviewed — see +Your `confidence` decides whether your comment reaches the bug unreviewed. See **Recording actions** in the system prompt before you pick a level. This is the -run's confidence in the _localization_; the severity block has its own, separate +run's confidence in the _localization_; the severity suggestion has its own, separate confidence (see `severity-assessment`). - **High** (you found the specific code and the cause is clear): record the 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 4f7e2e84a7..4f75682fe6 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 @@ -1,7 +1,7 @@ # Severity assessment Assess an appropriate Mozilla severity for the bug. Record it in **both** the -`severity_assessment` structured-output object and the severity block at the end of your +`severity_assessment` structured-output object and the closing severity sentence of your comment (see **Severity in the comment** in the system prompt). Base the judgment on **user impact and reach** as evidenced by the bug report and the code you investigated — how badly the user is affected, how many users hit it, and whether a workaround exists. @@ -59,8 +59,8 @@ It decides whether the comment mentions severity at all: - **High** — impact is clear-cut (clearly cosmetic, or clearly a crash/data-loss). - **Medium** — the level is a reasonable read but the impact or reach is arguable. -- **Low, or you could not assess it** — **omit the severity block from the comment - entirely**, horizontal rule included, and set `confidence` accordingly (or the whole +- **Low, or you could not assess it** — **omit the severity sentence from the comment + entirely**, and set `confidence` accordingly (or the whole `severity_assessment` object to null). Say nothing rather than guess: a level you are unsure of still reads as a judgment an engineer may act on, and being wrong there costs trust in the rest of your comment. diff --git a/agents/frontend-triage/tests/test_hooks.py b/agents/frontend-triage/tests/test_hooks.py index cb8849b918..9e38d8d8c3 100644 --- a/agents/frontend-triage/tests/test_hooks.py +++ b/agents/frontend-triage/tests/test_hooks.py @@ -84,7 +84,7 @@ def test_a_refused_comment_does_not_use_up_the_allowance(): def test_a_comment_may_declare_its_severity_only_once(): plan = "Root cause is a stale selector in content-area.css." - block = "\n\n---\n\nSuggested severity: S4\nCosmetic only, no functional impact." + block = "\n\nSuggested severity: S4. Cosmetic only, no functional impact." # Absent is deliberate — a run with low severity confidence omits the block. severity_block_hook({"params": {"text": plan}})