I sell a human second pass on one AI-written PR (Riven Desk). Before pitching that, I wanted to do the work in public: pick three recent agent PRs from real repos, read the diffs (not just the summaries), and apply the same STOP checklist I give away for free.
Method, briefly:
- Searched public GitHub for PRs authored by
copilot-swe-agentand bodies/trailers mentioning Claude Code (Co-Authored-By: Claude/claude.com/claude-code). - Preferred medium diffs (under ~400 changed lines) in repos people might recognize.
- Reviewed each against the STOP conditions one-pager — when to refuse the merge even if CI is green.
These are outsider reviews. I don't maintain these projects. Maintainers may have context I don't. I'm grading the diff as written, not the people.
PR 1 — microsoft/testfx#11740 (Copilot)
PR: Deduplicate sample binlog argument construction
Author signal: GitHub Copilot coding agent (copilot-swe-agent)
Size: ~27 changed lines across eng/build-samples.ps1, eng/samples-tools.ps1, eng/test-samples.ps1
State when reviewed: merged
What it does
Extracts repeated “build a -bl: / /bl: path under $BinaryLogDirectory” into Get-SampleBinlogArgument in eng/samples-tools.ps1, then calls it from the sample build/test scripts. The call sites already dot-source samples-tools.ps1, so the helper is in scope.
STOP checklist
| STOP | Fires? | Notes |
|---|---|---|
| 1 Secrets | No | No credentials or env files |
| 2 Blast radius / no boundary | No | One clear intent: dedupe binlog arg construction |
| 3 Mixed concerns | No | Script-only, no lockfile/infra hitchhikers |
| 4 “No behavior change” while surface moved | Borderline | Behavior should match; see nit below |
| 5 Rollback story | Fine | One revert undoes it |
| 6 Security-sensitive paths | No | Build helper only |
| 7 Prompt/tool surface | No | |
| 8 CI / tests | N/A from diff alone | Trivial pure helper; no new failing assertion added |
Concrete findings
-
Default prefix is
-bl:(Get-SampleBinlogArgument). Call sites that need MSBuild-style/bl:pass-ArgumentPrefix "/bl:"explicitly. That looks correct in the diff — just something a human should eyeball once so a future caller doesn’t assume the wrong flag. -
Log name vs extension: the helper always appends
.binlogto$LogName. Call sites that previously built"$name.binlog"now pass$name(or"$name.restore"). Consistent in this PR; don’t re-add.binlogin the caller later. - No unit test for the helper. For this size I’d accept it — the risk is a wrong path string, and the scripts are the real consumers.
Merge stance
Would merge. Nothing on the STOP list fires hard. This is the kind of agent PR that should land with a short human glance, not a drama review.
What I’d fix before merge (optional): one sentence in the PR body: “Default prefix -bl:; MSBuild restore/build paths pass /bl:.” Saves the next reviewer two minutes.
PR 2 — trimble-oss/modus-wc-2.0#1569 (Copilot)
PR: Update vulnerable dependency pins
Author signal: GitHub Copilot coding agent
Size: package.json + package-lock.json (~280 line churn, mostly lockfile)
State when reviewed: open
What it does
Updates npm overrides / pins for brace-expansion@1|2|5 and fast-uri, and bumps @stencil/react-output-target from 1.2.0 → 1.6.2 (lockfile follows, including @lit/react, ts-morph, nested minimatch, etc.).
STOP checklist
| STOP | Fires? | Notes |
|---|---|---|
| 1 Secrets | No | |
| 2 Blast radius / no boundary | Yes — ask/split | Title says vulnerable pins; diff also jumps a codegen package several minors |
| 3 Mixed concerns | Yes — split | Security pin refresh + Stencil React output-target upgrade in one PR |
| 4 Surface moved | Ask | React wrapper generation can change across 1.2→1.6 with no app source in the diff |
| 5 Rollback | Partial | Revert works; “why these versions” isn’t written |
| 6 Security paths | Skimmed | Dependency pins are security-adjacent — need the CVE/advisory names in the PR |
| 7 Prompt/tool | No | |
| 8 Tests that catch the regression | Ask | Lockfile-only PRs often go green without proving consumers still build |
Concrete findings
-
@stencil/react-output-target1.2.0 → 1.6.2 is not a pin tweak. Peer dependency text in the lockfile widens toward Stencil 5. That can change generated React bindings. I’d want either (a) that bump in its own PR with a smoke build of the React output, or (b) a short note linking the release notes and what was verified. -
brace-expansion/fast-urioverrides look like the actual “vulnerable pins” work. Fine — but the PR body should name the advisories (or Dependabot/Snyk findings) so a reviewer isn’t trusting the title alone. I didn’t see CVE IDs in the title; treat that as missing evidence, not proof they’re wrong. -
Lockfile-only confidence: no source/test changes. Merge only if CI already builds the React/Angular output targets you ship, or after a manual
npm runof those packages.
Merge stance
Would not merge as written — request changes / split.
Smallest clear path:
- Split: PR A = brace-expansion + fast-uri pins only. PR B =
@stencil/react-output-targetupgrade with a one-line verification note. - Or keep one PR, but add: advisory links for the pins, changelog pointer for 1.2→1.6, and “I built X output target locally / CI job Y is green.”
This is a classic agent shape: honest security cleanup, then a larger upgrade rides along because the agent “fixed versions” broadly.
PR 3 — Asymptote-Labs/agent-beacon#723 (Claude Code)
PR: feat(lenses): built-in MCP Calls lens
Author signal: human opener + Co-Authored-By: Claude / claude.com/claude-code markers
Size: ~387 changed lines — new mcp.lens.html, Playwright e2e, fixture lines, docs
State when reviewed: open
What it does
Adds a built-in dashboard “MCP Calls” lens: group MCP tool calls by server/tool, show args/results, mark failures, document it in docs/concepts/lenses.mdx, and cover it with Playwright (builtin-mcp.spec.ts) including an XSS-shaped payload in fixture args.
STOP checklist
| STOP | Fires? | Notes |
|---|---|---|
| 1 Secrets | No | |
| 2 Blast radius | No | Matches “add MCP lens” intent |
| 3 Mixed concerns | No | Feature + tests + docs for the same lens |
| 4 Surface claim | No | Docs say seven built-in lenses now |
| 5 Rollback | Fine | Revert removes the lens file + docs line |
| 6 Security-sensitive | Reviewed | Renders untrusted trace payloads in the browser |
| 7 Prompt/tool / rendered AI output | Watched closely — clears | Uses textContent / el() helpers; e2e asserts markup in args does not execute |
| 8 Tests | Strong | Playwright checks grouping, failure flag, XSS non-execution, empty state |
Concrete findings
-
XSS handling looks intentional and tested. Comment in the lens: arguments/results are untrusted. Fixture plants
<img src=x onerror=...>; the test opens Arguments and expectswindow.pwnedto stay undefined. That’s the right bar for a lens that prints agent/MCP payloads. -
Field-shape ask (not a block if CI is green): the Playwright fixture puts
arguments/resultundergen_ai.tool.call, while the lens JS readsevent.tool.arguments/event.tool.resultandevent.tool_call_id. Ifwindow.beacon.getTrace()normalizes those fields before the lens runs, fine — and the e2e implies it does. If someone later feeds raw JSONL into the lens, args/results would silently go missing. Worth one maintainer sentence: “getTrace maps gen_ai.call → tool.*.” -
Classification heuristics (
mcp__server__tool,MCP:tool,event.type === "mcp") are documented enough in empty-state copy. Edge cases (weird tool names) are acceptable for v1.
Merge stance
Would merge after confirming the e2e job that runs builtin-mcp.spec.ts is green on the PR. I would not block on style. I would leave the field-mapping note as a non-blocking comment.
This is closer to what you want from an agent: feature-sized, security-aware rendering, and a test that would fail if someone “helpfully” switched to innerHTML.
What I’d actually have blocked
Across these three:
| PR | Block? | Why |
|---|---|---|
| testfx#11740 | No | Small, matched intent, easy revert |
| modus-wc#1569 | Yes (as packaged) | Security pins mixed with a multi-minor Stencil React target bump; missing advisory + verification notes |
| agent-beacon#723 | No (pending green e2e) | Untrusted output handled; tests watch the scary path |
The interesting failure mode wasn’t “AI can’t code.” It was scope creep inside a true-sounding title (vuln pins) and whether security-sensitive UI proves it doesn’t execute untrusted text.
CI green is not a STOP clear. Title confidence isn’t either.
Takeaways I’m keeping
- Read the file list before the summary. If the title says “pins” and a codegen package jumped 1.2→1.6, stop and split.
- For anything that renders agent/tool output, demand a textContent-style path and a regression test. agent-beacon did this; many agent UIs don’t.
- Tiny refactors from agents are often fine. Don’t invent risk on testfx-shaped PRs just because an agent wrote them.
- Name a human owner and a rollback line on anything non-trivial. Agents don’t get paged on Monday.
I used the free STOP one-pager while writing this: STOP conditions before you merge an AI agent PR.
If you want a second set of eyes on one hard agent PR
I’m running a founding price on a single human review of one AI-written PR: $49 for the first 5 (normally $99) → Agent PR Audit — founding offer.
You get concrete findings with file names, a merge stance, and what to fix — same shape as the sections above, on your PR. No fake “bugs down X%” claims. The point is fewer merges you can’t explain.
— Gunjit / Riven Desk
Top comments (0)