hackbot: collect feedback on the comments agents post - #6451
Draft
crythms wants to merge 21 commits into
Draft
Conversation
The frontend-triage agent posts root-cause analyses to Bugzilla, but there was no way to tell whether they were any good. The people best placed to judge — the reporter, the module owner who'd take the patch — are often not Mozilla employees, so the rating surface has to be reachable without a Mozilla account. Comments from opted-in agents now carry a signed link to a public page in hackbot-ui, exempted from the SSO middleware. Ratings land in a new run_feedback table alongside per-field labels keyed to the agent's own structured output (root_cause, proposed_fix, target_files, confidence, actionable), so a thumbs-down aggregates into something actionable rather than prose. The link is appended at apply time rather than where the agent records the comment: only comments that actually reach Bugzilla should advertise a feedback URL, and the signing secret belongs to hackbot-api rather than the agent container. Bot resistance is load-bearing here, not a refinement. The URL lands in a public, heavily-crawled Bugzilla page and in bugmail to every CC'd account, where corporate mail scanners pre-fetch every link they see. So a GET never writes — the ?v= param only pre-selects a thumb — and the write requires a nonce minted when the page renders, which a client that never rendered it cannot produce. A per-run cap and partial unique indexes bound what's left. Feedback is off by default per agent and enabled only for frontend-triage; unset secret or base URL omits the footer entirely rather than posting a broken link.
Asking a passer-by on a Bugzilla page to rewrite a technical root-cause analysis was always the least likely thing in the schema to be used; a rater who has a correction will put it in the comment. Removing it before the migration runs anywhere.
Sign-in would have meant holding a live Bugzilla API key that can act as the user, plus a privacy review, in exchange for weighting and dedupe that only pay off at volumes this is unlikely to see soon. Not building it, so the columns go too. Revisitable later as one small migration. rater_kind stays: it's written on every row and is what separates human votes from anything that gets past the nonce.
Cuts 41 cases to 29. Removed: a second HMAC-mismatch case dressed as a different secret; is_enabled's base-url clause, already covered behaviourally by the footer tests; two nonce cases at the route layer that only re-prove what the links tests establish; an unknown-agent case that can't happen since run.agent comes from the registry. Trimmed parametrize lists that repeated one branch with different values. Nothing that covers a distinct branch or a stated guarantee was removed.
Three changes that belong together. The public pages move from /feedback to /rate. The SSO exemption in middleware.ts is a prefix match, so a page added at /feedback would have been silently public — putting the public surface in its own namespace means everything under /feedback stays guarded by the default matcher instead of relying on a narrower pattern. It also shortens the URL that lands in Bugzilla. Rater dedupe now keys on a per-browser cookie, falling back to the IP and user agent hash. That fallback alone was unsafe: two people behind one office or VPN egress IP on the same Firefox build hashed identically, and because the write is an upsert the second would silently overwrite the first. Losing a module owner's verdict to a colleague's later one is exactly the signal this is meant to collect. A cross-run review page at /feedback lists every rating with its comment, filterable by agent, backed by a new GET /feedback. Run detail gains a tally linking into it. Only agents with feedback links enabled can accumulate ratings, so an empty result is also how a non-participating agent renders — no flag needs plumbing to the UI.
The SSO-gated pages need a Google session, which makes them impossible to open against sample data without real OAuth credentials. DEV_AUTH_EMAIL stands in for one: honoured by devUser() in lib/session.ts and by an inlined copy of the same check in middleware.ts, which cannot import that module since it is server-only and middleware runs on the edge runtime. Both are guarded on NODE_ENV, so the branch is dead code in any production build. Verified by removing the variable: /feedback redirects to /login, /api/feedback returns 401, and only /rate/* stays reachable. The variable itself belongs in .env.local, which is gitignored, so nothing that activates this is committed.
The global input rule sets width:100%, which a checkbox obeys — it took the full row and pushed its label to the far edge, staggered per row. Both checkboxes in the app were affected; the one in TriggerForm only shows for build-repair, so it had gone unnoticed. Fixed at the global rule rather than in feedback scope, so the next checkbox added doesn't hit it. The rating page also showed the comment's trailing "add a needinfo" footer, which is redundant and faintly contradictory on a page whose whole purpose is to collect that correction. Trimmed from the preview only; what Bugzilla receives is unchanged.
The /feedback -> /rate move updated lib/hackbot.ts and the route directories but missed the client-side fetch in FeedbackForm, so Submit posted to /api/feedback/:token, which no longer exists. It 404'd every time. Two things hid it. The endpoint tests exercised /api/rate directly rather than the URL the component builds, and on a 404 res.json() threw on the HTML error page before the res.ok check, so the user saw a JSON syntax error rendered as small muted text — a hard failure that read as the button doing nothing. Errors now use the same error-banner the rest of the app uses, and a non-JSON body degrades to the status line.
The heading and preamble were rendered by the page, outside the form, so after submitting they stayed on screen — leaving "Was this analysis useful?" and an invitation to rate above a note saying the rating was already recorded. Moving them into FeedbackForm puts the entire panel under the component's own submitted state, so success shows the confirmation and nothing else, at 18px semibold.
Whether we recognised a repeat rater is bookkeeping, not something worth telling them about — and "updated" invites the question of what we know about them. Both paths now answer identically, which also collapses the write to a single return. Dedupe is unchanged: a repeat from the same browser still replaces the earlier verdict rather than stacking a row. The test now asserts that structurally, via the rollback and update counts, rather than by reading the message.
The list read as a stream, which is the wrong shape for scanning a set of structured records. It is now a table — rating, agent, bug, run, what was wrong, comment, when — reusing the runs table's styling, with dimensions as tags rather than a joined string. Two filters: clickable thumb toggles carrying live counts, and an agent select. Both apply client-side against the full result set, which also fixes the agent list: it was derived from the rows on screen, so it only appeared with two or more agents and would have emptied itself the moment you used it.
A comment is free prose of unpredictable length; a column wide enough for it starves every other field, and one narrow enough to fit alongside them cannot show it. Same trade-off run errors have, so it takes the same shape: the record's own fields stay on one line, the comment spans the table beneath it with a left rule tying the two together. Unlike the error row this is not line-clamped. An error preview is truncated because a stack trace could fill the page and the full text is one click away; a rater comment is the thing you opened the page to read, and the write path already caps its length.
The bug is reachable in one hop through the run, so the column was paying for itself twice; removing it gives "What was wrong" 44% of the width. Pagination follows RecentRuns exactly — 50 a page, a Load more button, filters held in the URL and applied server-side so they compose with paging. That also settles where the agent options come from: AGENT_NAMES in lib/agents.ts, the same source the runs filter uses. Deriving them from the loaded rows would have gone wrong again as soon as a page boundary hid an agent. Adds a rating filter to GET /feedback, which the thumb toggles now drive.
Two things came from copying RecentRuns too literally. Its agent filter is built from AGENT_NAMES, the whole registry, which is right there and wrong here: feedback links are opt-in per agent, so most of that list can never appear on this page. The options now come from the agents that have actually been rated. The thumb toggles also lost their counts when paging arrived, because counting loaded rows would only ever describe one page. A new GET /feedback/stats returns totals broken down by agent in one query, which drives both the counts and the option list. It deliberately takes neither an agent nor a rating parameter — narrowing by either would collapse the list it exists to populate, which is how the client-side version of this filter managed to erase itself. Counts follow the agent filter, so each number is how many rows clicking that thumb would show.
Filters on the dimension labels via JSONB containment, so it composes with the agent, rating and run filters already there. The options are the full fixed vocabulary rather than only labels that have been used — the opposite of the agent filter, and for a reason: an agent without a feedback link can never appear, whereas any dimension is one rating away, so an empty result is an answer rather than a dead end. Tested by compiling the statement and asserting the containment clause is present, since the fake DB in this suite never executes SQL — a filter that silently failed to apply would otherwise read as "no rows match".
Comments: 126 lines to 109. Cut docstrings that restated their own code (is_enabled, verify_token), the second half of anon_id's, and the third copy of the /rate-vs-/feedback namespace rule — it now lives in middleware.ts, where the risk is, with pointers from the two callers. Dropped a couple of asides that narrated how the code got here rather than what it does. Tests: 40 to 35. Removed one that duplicated an assertion already made in the test above it, one asserting a filter is absent when not requested, one covering dict.get returning None, and one proving at the route layer what a unit test already proves about anon_id. Folded a stats assertion into its neighbour rather than losing it.
Reverts the DEV_AUTH_EMAIL escape hatch now that the UI review is done. It was provably inert in production — verified by running a production build with the variable set, where /feedback still redirected to /login and /api/feedback still returned 401 — but nothing that exists only to serve a local harness should ship, and an auth bypass is the last thing worth asking a reviewer to reason about. lib/session.ts is now identical to master; middleware.ts differs only by the /rate exemption the public rating pages need.
The Actions preview rendered params.text, the comment as the agent recorded it, while the rating footer is appended at apply time and never persisted. So the preview never matched what Bugzilla received, and someone who triggered the run had no way to rate their own result from Hackbot — they had to open the bug. RunActionDoc now carries posted_text: the exact string sent to Bugzilla, derived on read via the same helper the applier uses. Nothing is stored, so a re-apply still cannot stack a second copy of the link. Only populated once the action is applied. The rating page rejects a comment that was never posted, so a link offered before Apply would 404 — before that the preview stays the agent's text, which is what there is to review at that point.
The public rating link only works once a comment reaches Bugzilla, which
misses the judgment that matters most: a reviewer reading an analysis in
Hackbot, deciding it is wrong, and declining to post it at all. That is a
clearer signal than anything a drive-by rater leaves on a live bug.
POST /runs/{run_id}/feedback backs a rating control on the run page. It
differs from the public route in one flag on the shared lookup —
applied_only=False — so a comment that was only ever recorded can still be
judged. The public path keeps the stricter guard: an outsider can only rate
what Bugzilla actually received.
Internal ratings are attributed. The email comes from the session via
X-On-Behalf-Of, the convention requested_by already uses, and never from
the request body, so a client cannot rate as someone else. Dedupe moves to
a second partial index on (run_id, rater_id), and RaterKind becomes
anonymous | mozilla — "bugzilla" was dead once sign-in was dropped, and the
distinction worth keeping is reviewer-before-posting versus public-after.
Both surfaces are gated on one registry flag via rating_enabled(), so an
agent opts in by setting feedback_link on its AgentSpec and nothing else.
Only frontend-triage has. Actions carry can_rate so the control is hidden
for agents that have not, before Apply as well as after.
The preview keeps showing the agent's text rather than the posted comment.
Rendering the Bugzilla footer here would offer a second, anonymous rating
path beside the attributed one, and the two do not dedupe against each
other. posted_text stays on the API as the record of what was sent.
The migration is amended in place rather than stacked, since it has not
been applied anywhere yet.
Audits the comments added on this branch the same way the PR description was audited: for claims stated as fact that were inference. The prevalence claims go. "Bugmail reaches every CC'd account and corporate mail scanners pre-fetch every link they see" was a threat model written as an observation — I never checked that Mozilla bugmail passes through such a gateway, nor that it is every link. It now says the design assumes automated clients follow the link, which is the part that justifies the nonce. Likewise "most recorded comments are never applied" becomes the fact behind it: applying is a separate, currently manual step. One comment was simply wrong. The thumb counts were documented as "always how many rows clicking it would show", but stats take no dimension parameter, so with a dimension selected the thumb reads 48 where the rows are 6. The comment now says what the code does. Also drops rationale duplicated between the TypeScript mirrors and the pydantic models they mirror, and shortens the rest.
38 cases, down from 41. The applied-only contrast already asserts the 404 that a separate test re-made one call deeper. The stats breakdown covers the "only rated agents" claim by construction, so giving one agent a single rating folds the zero-fill case in with it. And posted_text's presence and absence are two arms of one `if`, so they read better as one test. No branch or stated guarantee lost coverage.
crythms
requested review from
a team and
msujaws
and removed request for
msujaws
August 2, 2026 14:29
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
1 participant
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.
We have no good way to tell whether frontend-triage's Bugzilla comments are good enough. This
adds two ways to say so: a rating control on the run page, and a 👍/👎 link in
the posted comment.
The run-page one works before Apply, so a reviewer who decides not to post a
comment still records why. The Bugzilla one needs no login, since Bugzilla is
public and many of the people reading these comments have no Mozilla account.
Thumbs-down offers checkboxes matching the agent's own output fields
(
root_cause,proposed_fix, …), so a rejection aggregates per-field.Opt-in per agent: set
feedback_linkon anAgentSpecand both surfacesswitch on together. Only
frontend-triagehas it.Design choices
Public ratings are anonymous; ratings made in Hackbot carry the reviewer's
email. Bugzilla auth delegation would attribute the public ones too, but it
returns an API key that authenticates as that user for any Bugzilla call, which
I didn't want this service holding.
rater_kindkeeps the two kindsdistinguishable in the data.
The public page assumes automated clients will follow the link. Mail
gateways and chat clients fetch links to scan or preview them, so a GET never
writes —
?v=only pre-selects a thumb — and the write requires a nonce mintedwhen the page renders. Anonymous ratings are capped per run, and the page is
marked
noindex, nofollowvia a meta tag.Nothing reads the ratings back yet. This PR only collects them. How they
feed into improving the agent depends on what people actually write, which we
don't know until some of it exists.
Deploying, in order
Alembic
d7e2f9a4c1b3first — the endpoints need the table. Additiveonly. Please check
alembic currentshowsf3c8a1d5b2e7: the revision wasamended in place on the assumption it had never run.
Deploy. Safe alone — without the env vars below no links are emitted, and
the run-page control still works, so you can shake it down before anything
is public.
Set on hackbot-api, last, since this starts putting links into permanent
public comments:
The secret must never rotate: tokens are HMACs over run ids, so a new key
would 404 every link already published.