From 07d183c9d128f50b71e605b7c68fe304dd799305 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Sat, 31 Oct 2026 15:00:24 +0100 Subject: [PATCH 1/2] fix(pr-review): review interface code for correctness, with UI recall eval cases The reviewer scored UI pull requests 10/10 after checking only that class names resolved, while another reviewer found real defects on the same heads (#2015, #1950, #1972). The prompt gains an "Interface code" section: edits lost on one way out, text wrong for a reachable state, accessible names that drop information, layout arithmetic at the narrowest viewport, and client copies of server rules. The eval corpus gains those misses as cases: #2015 and #1950 in development (the section was written from them) and #1972 as a holdout. --- apps/ai/scripts/pr-review-eval/README.md | 7 + apps/ai/scripts/pr-review-eval/corpus.json | 184 +++++++++++++++++++++ apps/ai/src/chat/prompts.ts | 8 + 3 files changed, 199 insertions(+) diff --git a/apps/ai/scripts/pr-review-eval/README.md b/apps/ai/scripts/pr-review-eval/README.md index 3c99c5bce2..4f103a3665 100644 --- a/apps/ai/scripts/pr-review-eval/README.md +++ b/apps/ai/scripts/pr-review-eval/README.md @@ -123,6 +123,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. diff --git a/apps/ai/scripts/pr-review-eval/corpus.json b/apps/ai/scripts/pr-review-eval/corpus.json index 4ae7cd1825..e6a6c9703b 100644 --- a/apps/ai/scripts/pr-review-eval/corpus.json +++ b/apps/ai/scripts/pr-review-eval/corpus.json @@ -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", + "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`." } ] } diff --git a/apps/ai/src/chat/prompts.ts b/apps/ai/src/chat/prompts.ts index 435e66cc93..1a2996e451 100644 --- a/apps/ai/src/chat/prompts.ts +++ b/apps/ai/src/chat/prompts.ts @@ -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 +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. From 5d23c9735352b65185e00e7ddafa36555188c0a5 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Sat, 31 Oct 2026 15:31:28 +0100 Subject: [PATCH 2/2] fix(pr-review): child reviewers get the interface checks; eval README fetches pinned PR heads Over 12 files the parent delegates groups to child reviewers and does not re-read their diffs, so the interface-code checks have to be in the worker prompt too. The eval README now fetches the PR heads the corpus pins, so a fresh clone can run it. --- apps/ai/scripts/pr-review-eval/README.md | 3 +++ apps/ai/src/chat/review-fanout.ts | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/ai/scripts/pr-review-eval/README.md b/apps/ai/scripts/pr-review-eval/README.md index 4f103a3665..4691878523 100644 --- a/apps/ai/scripts/pr-review-eval/README.md +++ b/apps/ai/scripts/pr-review-eval/README.md @@ -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 diff --git a/apps/ai/src/chat/review-fanout.ts b/apps/ai/src/chat/review-fanout.ts index 9c35279b1d..2f6679bd35 100644 --- a/apps/ai/src/chat/review-fanout.ts +++ b/apps/ai/src/chat/review-fanout.ts @@ -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 -)