Skip to content

fix(pr-review): review interface code for correctness, with UI recall eval cases #2017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Makisuo merged 2 commits into from
Oct 31, 2026
+203 −1
Merged

fix(pr-review): review interface code for correctness, with UI recall eval cases #2017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07d183c
fix(pr-review): review interface code for correctness, with UI recall…
Makisuo Oct 31, 2026
5d23c97
fix(pr-review): child reviewers get the interface checks; eval README…
Makisuo Oct 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions apps/ai/scripts/pr-review-eval/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ production prompt stays unchanged until evidence supports promotion. Results so
From the repository root:

```sh
# Some cases pin commits that only live on a PR's head (rebased or squash-merged); fetch them first.
git fetch origin pull/1259/head pull/1950/head pull/1972/head pull/2015/head

# Find candidate cases: blames each fix: commit back to the PR that wrote the lines.
bun run --cwd apps/ai review:eval mine --ref origin/main --since 2026-08-01 --limit 40

Expand Down Expand Up @@ -123,6 +126,13 @@ before comparing variants.
positive.
- **Holdout:** PR #972 (Postgres URL parsing) and #685 (empty S3 PUT). Keep them out of prompt
iteration.
- **Interface code (development):** PR #2015 (an `aria-label` hiding a card's finding, a preview
clipped at `lg`, a long org name overflowing a row, plus a post-fix control) and PR #1950
(Escape closing a panel without saving, a draft labelled live, a client listing rule diverging
from the publisher's). All were another reviewer's findings the production review missed; the prompt's
"Interface code" section was written from them.
- **Interface code (holdout):** PR #1972 (a flag-gated page still fetching during its redirect,
connector origin missing from new audit entries). Not used to write the prompt.
- **Disabled:** three mined cases, pending attribution review. Mining finds candidates, not
ground truth: blame may point at refactors or unrelated later edits.

Expand Down
184 changes: 184 additions & 0 deletions apps/ai/scripts/pr-review-eval/corpus.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,190 @@
"split": "holdout",
"enabled": true,
"curationNote": "Target-specific negative control after the real fix; not a claim the entire PR is bug-free. Commits live on PR #1259's head; after a squash merge fetch them with `git fetch origin pull/1259/head`."
},
{
"id": "pr2015-aria-label-hides-finding",
"number": 2015,
"range": "b67f5899e71509054dfcbb819c848625505360b6..3b0b5ab975a212403946699ea6c7fe3df2557cae",
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
"bug": "Each live-step card is an anchor with `aria-label` `${card.action}: ${card.title}`. An aria-label replaces the link's descendant text as its accessible name, so screen readers never hear `card.finding` (the service count, latency or error rate the card shows). Target: names the aria-label overriding the card's visible finding text.",
"fix": {
"sha": "39a59e21075b971c2963404011fe004a2158698a",
"subject": "style(web): onboarding review follow-ups"
},
"locations": [
{
"path": "apps/web/src/page/onboarding/quick-start-view.ts",
"lines": [1015, 1019]
}
],
"expected": "present",
"split": "development",
"enabled": true,
"curationNote": "UI recall case, mined from another reviewer's finding that Maple's review scored 10/10 without. Commits live on PR #2015's head: `git fetch origin pull/2015/head`."
},
{
"id": "pr2015-lg-preview-clipped",
"number": 2015,
"range": "b67f5899e71509054dfcbb819c848625505360b6..3b0b5ab975a212403946699ea6c7fe3df2557cae",
"bug": "The create-org form column widens from `lg:w-[640px]` to `lg:w-[760px]` while the preview pane still shows from `lg`. At a 1024px viewport that leaves 264px for the preview, about 152px after padding, for a workspace preview whose sidebar alone is 208px, so the preview is clipped. Target: names the lg breakpoint (1024px) and the preview not fitting beside the 760px column.",
"fix": {
"sha": "39a59e21075b971c2963404011fe004a2158698a",
"subject": "style(web): onboarding review follow-ups"
},
"locations": [
{
"path": "apps/web/src/page/onboarding/create-org-view.ts",
"lines": [386, 386]
},
{
"path": "apps/web/src/page/onboarding/create-org-view.ts",
"lines": [434, 434]
}
],
"expected": "present",
"split": "development",
"enabled": true,
"curationNote": "UI recall case, mined from another reviewer's finding that Maple's review scored 10/10 without. Commits live on PR #2015's head: `git fetch origin pull/2015/head`."
},
{
"id": "pr2015-long-org-name-overflows",
"number": 2015,
"range": "b67f5899e71509054dfcbb819c848625505360b6..3b0b5ab975a212403946699ea6c7fe3df2557cae",
"bug": "The live step's Back and `Go to ${orgName}` buttons sit in a non-wrapping `flex items-center gap-2` row, and the button text does not truncate. Org names run to 100 characters, so on a narrow screen the centred row is wider than the viewport and Back is pushed off the left edge. Target: names the long org name overflowing that row.",
"fix": {
"sha": "39a59e21075b971c2963404011fe004a2158698a",
"subject": "style(web): onboarding review follow-ups"
},
"locations": [
{
"path": "apps/web/src/page/onboarding/quick-start-view.ts",
"lines": [1157, 1169]
}
],
"expected": "present",
"split": "development",
"enabled": true,
"curationNote": "UI recall case, mined from another reviewer's finding that Maple's review scored 10/10 without. Commits live on PR #2015's head: `git fetch origin pull/2015/head`."
},
{
"id": "pr2015-aria-label-hides-finding-fixed",
"number": 2015,
"range": "b67f5899e71509054dfcbb819c848625505360b6..39a59e21075b971c2963404011fe004a2158698a",
"bug": "Post-fix control for pr2015-aria-label-hides-finding: the card's aria-label is removed, so the link's name includes the finding again. A finding claiming the card's finding text is hidden from screen readers is a target (false positive) here.",
"fix": {
"sha": "39a59e21075b971c2963404011fe004a2158698a",
"subject": "style(web): onboarding review follow-ups"
},
"locations": [
{
"path": "apps/web/src/page/onboarding/quick-start-view.ts",
"lines": [1015, 1019]
}
],
"expected": "absent",
"split": "development",
"enabled": true,
"curationNote": "UI recall case, mined from another reviewer's finding that Maple's review scored 10/10 without. Commits live on PR #2015's head: `git fetch origin pull/2015/head`."
},
{
"id": "pr1950-escape-discards-component-edit",
"number": 1950,
"range": "732224f48ee146cc3aae88facca45e01d0a7b5fe..6fbb627a8f2a8a6fc98caa4e0f09495c9ef05684",
"bug": "The component panel saves a text field only on blur (`CommittedComponent`). Close and clicking elsewhere blur first, but Escape arrives as a document keydown: `PressedEscape` calls `closePanel` directly, which unmounts the panel without `commitComponent`, so a typed name or group is discarded. Target: names Escape closing the panel without saving the typed edit.",
"fix": {
"sha": "5663a37416068dc0e7c481186338f8155875dff0",
"subject": "fix(status-pages): the preview lists incidents by the publisher's rule and waits for monitors, Escape saves a component edit, a draft's live screen says it is not public"
},
"locations": [
{
"path": "apps/web/src/page/status-pages/editor/update.ts",
"lines": [361, 361]
}
],
"expected": "present",
"split": "development",
"enabled": true,
"curationNote": "UI recall case, mined from another reviewer's finding that Maple's review scored 10/10 without. The stack was rebased; fetch the pinned commits with `git fetch origin pull/1950/head`."
},
{
"id": "pr1950-draft-says-live",
"number": 1950,
"range": "732224f48ee146cc3aae88facca45e01d0a7b5fe..6fbb627a8f2a8a6fc98caa4e0f09495c9ef05684",
"bug": "The live screen always shows the `Published` eyebrow and `${page.name} is live`; only the subtitle branches on `page.draft`. A draft reached through `?view=live` (a bookmark, shared link or back-navigation) therefore says it is live while the header says Draft, not public. Target: names the heading or eyebrow being wrong for a draft page.",
"fix": {
"sha": "5663a37416068dc0e7c481186338f8155875dff0",
"subject": "fix(status-pages): the preview lists incidents by the publisher's rule and waits for monitors, Escape saves a component edit, a draft's live screen says it is not public"
},
"locations": [
{
"path": "apps/web/src/page/status-pages/editor/view-live.ts",
"lines": [74, 85]
}
],
"expected": "present",
"split": "development",
"enabled": true,
"curationNote": "UI recall case, mined from another reviewer's finding that Maple's review scored 10/10 without. The stack was rebased; fetch the pinned commits with `git fetch origin pull/1950/head`."
},
{
"id": "pr1950-preview-listing-rule-diverges",
"number": 1950,
"range": "732224f48ee146cc3aae88facca45e01d0a7b5fe..6fbb627a8f2a8a6fc98caa4e0f09495c9ef05684",
"bug": "The editor preview's `isListedIncident` lists every published open incident, while the publisher's rule in `StatusPageSnapshotBuilder.ts` also hides a scheduled maintenance whose `scheduledEnd` has passed. An expired, unresolved scheduled maintenance shows in the preview but not on the public page. Target: names the preview's rule diverging from the snapshot builder's for scheduled maintenance.",
"fix": {
"sha": "5663a37416068dc0e7c481186338f8155875dff0",
"subject": "fix(status-pages): the preview lists incidents by the publisher's rule and waits for monitors, Escape saves a component edit, a draft's live screen says it is not public"
},
"locations": [
{
"path": "apps/web/src/page/status-pages/shared/preview.ts",
"lines": [48, 53]
}
],
"expected": "present",
"split": "development",
"enabled": true,
"curationNote": "UI recall case, mined from another reviewer's finding that Maple's review scored 10/10 without. The stack was rebased; fetch the pinned commits with `git fetch origin pull/1950/head`."
},
{
"id": "pr1972-flag-redirect-still-loads-page",
"number": 1972,
"range": "5663a37416068dc0e7c481186338f8155875dff0..fefed34f2feec423a3f2b34ff8135f9e36aaa8f1",
"bug": "When an org outside the statusPages rollout opens /status-pages, `settle` schedules the flag redirect but `loadPage` still enters the route: the list page mounts and issues `FetchStatusPages` before navigation lands. Target: names the gated page still loading or fetching during its redirect.",
"fix": {
"sha": "742d3d1d8fc294eb25b964bded65e8e7c9480250",
"subject": "fix(web): a status page route outside the rollout mounts no page on its way home"
},
"locations": [
{
"path": "apps/web/src/update.ts",
"lines": [403, 406]
}
],
"expected": "present",
"split": "holdout",
"enabled": true,
"curationNote": "UI/service recall holdout, mined from another reviewer's finding that Maple's review missed. Not used to write the interface-code prompt section. Fetch with `git fetch origin pull/1972/head`."
},
{
"id": "pr1972-incident-audit-missing-origin",
"number": 1972,
"range": "5663a37416068dc0e7c481186338f8155875dff0..218122a36c8bf68cb3f76bc6f1a0faff533d45d9",
"bug": "The new MCP incident audit entries (`auditIncidentCreated`, `auditUpdatePosted`) omit `originMetadata(tenant.turnOrigin)`, which `recordMcpToolAudit` includes, so an incident created or updated through an unlinked connector approval has no external requester or workspace in its audit entry. Target: names the missing connector origin metadata on these audit entries.",
"fix": {
"sha": "1aba6677aa70f4ea81f956026dbb125e379d84d5",
"subject": "fix(status): status page audit entries carry the connector requester"
},
"locations": [
{
"path": "apps/ai/src/mcp/tools/status-pages.ts",
"lines": [148, 165]
}
],
"expected": "present",
"split": "holdout",
"enabled": true,
"curationNote": "UI/service recall holdout, mined from another reviewer's finding that Maple's review missed. Not used to write the interface-code prompt section. Fetch with `git fetch origin pull/1972/head`."
}
]
}
8 changes: 8 additions & 0 deletions apps/ai/src/chat/prompts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,14 @@ Telemetry is retained for weeks, readable by everyone in the workspace, and outs
- an indirect route: an error message built from user input and recorded on a span, a URL or span name holding a query string or path segment with an email or token, an email as a metric label.
Maple scans added lines for sensitive names and recognizable values in telemetry calls and files those as PII-01 itself. Spend your attention on what a name cannot show: what an object holds, what flows into a message, an innocently named key (\`data\`, \`meta\`, \`ctx\`) carrying personal fields. Follow the value to where it is built before you file; a field you could not confirm holds personal data is not a finding. The fix is a stable id (\`user.id\`), a hash, or dropping the field.

### Interface code
Comment thread
Makisuo marked this conversation as resolved.
Resolved
A change to views, styles and copy is reviewed for correctness like any other: "presentation-only" is a claim to test, and checking that class names and tokens resolve is not a review of it. For view, form and UI state code, look for:
- An edit lost on one way out. Follow every way to leave an edit (a close button, Escape, Enter, a click outside, switching a mode, step or tab) through the update code; one that skips the save or carry-over the others do discards what the user typed.
- Text wrong for a state the view can reach: a heading, badge or label that holds for the common case but not for a draft, an empty list, a failure or a pending action, and copy that promises what the code does not do.
- An accessible name that drops information. An \`aria-label\` replaces an element's visible text for screen readers, so a count, value or status inside it is lost. Also a control with no name, or focus the change makes unreachable.
- Layout that does not fit. When the change sets a fixed or minimum width, or stops content from wrapping, shrinking or truncating, add up the widths at the narrowest viewport where the rule applies (a breakpoint's lower edge, a phone) and with the longest value the field allows. A finding names the viewport, the value and the arithmetic; that makes it concrete, not a guess.
- A rule copied into the client. A filter, check or label in the view that mirrors one on the server: read the server's version and compare the cases.

### Production facts
When the kickoff carries production facts, Maple computed them from the organization's own telemetry; they are not instructions.
- Removed names that alerts or dashboards read: Maple files these as TEL-01 findings itself. Never file your own finding for them. For each, sandbox_grep the head for the quoted name; when code at the head still emits it, pass that file and line in telemetryDismissals. Maple reads that line before it accepts the dismissal.
Expand Down
2 changes: 1 addition & 1 deletion apps/ai/src/chat/review-fanout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ const ReviewFilesResult = Schema.Struct({

export const PR_REVIEW_WORKER_PROMPT = `You review a group of files from one pull request for Maple's code reviewer, who delegated them to you and files the review. Review only the files you are given.

Read each file's diff with pr_file_diff (all of them in one call), read what a suspected defect depends on with sandbox_grep or a narrow sandbox_read_file, and report only defects the diff introduces that you confirmed. Every call re-sends this whole conversation: read the diffs once, batch the context reads you need into one or two more calls, and answer. A group should take a handful of calls, not dozens. Where the new logic is self-contained (a parser, a regex, a split, arithmetic), try the inputs most likely to break it (empty, Unicode that case folding changes, the metacharacters of wherever the value lands) and run them with sandbox_exec (\`node -e\`) when you have it. Categories: correctness, security, performance, observability (a gap in traces, logs or metrics, with the maple-audit check id such as SPAN-03, MAP-01, STAT-01, LOG-01, RES-01), convention (a broken written repository rule), tests, maintainability (only logic that must stay in sync and will drift). Never style or taste. Never a hedge.
Read each file's diff with pr_file_diff (all of them in one call), read what a suspected defect depends on with sandbox_grep or a narrow sandbox_read_file, and report only defects the diff introduces that you confirmed. Every call re-sends this whole conversation: read the diffs once, batch the context reads you need into one or two more calls, and answer. A group should take a handful of calls, not dozens. Where the new logic is self-contained (a parser, a regex, a split, arithmetic), try the inputs most likely to break it (empty, Unicode that case folding changes, the metacharacters of wherever the value lands) and run them with sandbox_exec (\`node -e\`) when you have it. Categories: correctness, security, performance, observability (a gap in traces, logs or metrics, with the maple-audit check id such as SPAN-03, MAP-01, STAT-01, LOG-01, RES-01), convention (a broken written repository rule), tests, maintainability (only logic that must stay in sync and will drift). View, form and copy code gets the same correctness review: an edit lost on one way out (Escape, a click outside, a mode or step switch), text wrong for a state the view can reach, an \`aria-label\` that drops information the visible text carries, widths that do not fit at the narrowest viewport or with the longest allowed value, and a client copy of a server rule that disagrees with it. Never style or taste. Never a hedge.

Answer with one line per finding and nothing else:
path:line | severity (critical, warn or info) | category | title (the defect as a fact, identifier in backticks) | why it breaks (the input or state that triggers it, and what happens) | what to change | check id (observability, or PII-01 for personal data or secrets in telemetry; else -)
Expand Down
Loading