Skip to content

feat(review-pr): require evidence for security and unwired-UI findings - #47

Draft
captainsafia wants to merge 1 commit into
mainfrom
oz/review-pr-evidence-rules
Draft

feat(review-pr): require evidence for security and unwired-UI findings#47
captainsafia wants to merge 1 commit into
mainfrom
oz/review-pr-evidence-rules

Conversation

@captainsafia

@captainsafia captainsafia commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Ran the update-pr-review skill over the last 7 days of warpdotdev/warp-server pull requests (100 PRs, 82 review threads, 13 agent review comments that drew human replies) and folded the repeated reviewer signals back into the shared review-pr skill.

All PRs in the window were code PRs (review_type: "code"), so review-spec is unchanged.

Signals

1. Speculative security findings get rejected (3 threads, 3 reviewers).

  • #13603⚠️ [IMPORTANT] [SECURITY] claimed logging raw errors could leak secrets. @seemeroland: "I think we need to be able to log errs. If it turns out something sensitive is actually included we should remove it from the err".
  • #13588 — same shape, sending a status message to RudderStack. @seemeroland: "I think this is fine".
  • #13615⚠️ [IMPORTANT] asked for encodeURIComponent on an agent UID. @johnturcoo: "agent IDs are uuid and uniform".

In each case the bot asserted a hazard from a value's hypothetical shape without tracing how the value is actually produced. The new rule asks for that trace before filing the finding, and routes unprovable concerns to top-level body instead of an asserted [SECURITY] inline comment.

This does not weaken genuine findings: the evidence-backed security comments in the same window were accepted (#13583 cross-team suite ownership — "Should be fixed now"; #13646 empty-owner authorization — "I think this is kind of valid. Will push a change to address").

2. Unwired UI elements are deliberate placeholders (3 threads, 2 PRs, 1 reviewer).

  • #13615 FactoryAgentEditor.tsx — Reconnect button with no onPress. @johnturcoo: "intentional".
  • #13601 index.tsx — New button with no onPress. "Intentional".
  • #13601 AgentActions.tsx — Copy-as-JSON menu item with no handler. "Intentional".

Three [IMPORTANT]/[SUGGESTION] comments, three identical dismissals. This extends the existing V0/initial-implementation rule to UI scaffolding.

Changes

Two bullets added to ## Review Scope in .agents/skills/review-pr/SKILL.md. No change to the output schema, severity labels, safety rules, or diff-annotation contract.

cc @captainsafia


Conversation: https://staging.warp.dev/conversation/dc738e5e-a8e5-4566-b2d4-29ee96789b37
Run: https://oz.staging.warp.dev/runs/019fb8b0-9544-7b70-9887-85d1d17593cc

This PR was generated with Oz.

Derived from a week of human replies to agent review comments on
warpdotdev/warp-server.

Reviewers rejected speculative sensitive-data and input-shape findings
("this error string could contain secrets", "this identifier could
contain a path separator") when the producing code made the hazardous
input impossible, and repeatedly answered "intentional" to IMPORTANT
findings about buttons and menu items that had no handler yet.

Co-Authored-By: Oz <oz-agent@warp.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant