Code review · review diff before PR merge

Feed the PR diff to Qwen3.8-flash; it gives a merge verdict and blocking / suggestion / nit findings, each with concrete impact and fix.

Hand the diff to the model; it returns a merge verdict and blocking / suggestion / nit findings.

This walkthrough uses a 6-file PR diff (new search endpoint + add caching + change payment settlement): hand the diff to qwen3.8-flash, ask for merge verdict, tiered findings (each with concrete consequence and fix), and a false-positive self-check list. The diff includes 7 test points, one of which is a decoy to test for false positives. The same input was run with thinking off and on to compare the two modes.

Call chain: qwen3.8-flash (6-file diff in one input → merge verdict + tiered findings + false-positive checklist). Thinking off suits pre-commit second-level screening; thinking on for a final pass before merge.

Steps: hand the PR diff to the model for review

Send the diff along with the prompt below to qwen3.8-flash. The diff below is excerpted for readability (the tested input is the full diff across 6 files). After excerpting, the input is shorter, so the number and wording of output items may differ from what is shown on this page. For reference only

You are a code reviewer. Below is the full diff of a PR about to be merged. Review and give your verdict.

Return only a JSON object, no other text, with this fixed structure:

{
  "verdict": "approve or request_changes",
  "summary": "Two or three sentences: what this PR does and why you give this verdict",
  "issues": [
    {
      "file":     "file path",
      "where":    "which function or which change",
      "severity": "blocking or suggestion or nit",
      "problem":  "what the issue is, one sentence",
      "impact":   "what concrete consequence it causes; don't write vague 'may be risky'",
      "fix":      "how to fix; provide code if possible"
    }
  ],
  "not_issues": [
    {"file": "file path", "looks_like": "what it looks like", "why_ok": "why it's not an issue based on this diff"}
  ],
  "missing_tests": ["tests that should be added but weren't; empty array if none"]
}

Review requirements:
- severity has only three levels, fixed meaning: blocking = can't merge without fix; suggestion = should fix but doesn't block merge; nit = style/taste.
  Don't rate style issues as blocking; don't rate real breakage as nit.
- Each issue's impact must state a concrete consequence (who sees what error, how data goes wrong); no vague risk descriptions.
- Judge based on actual diff content; don't guess from function names, file names, or code not shown.
- Spot cross-file issues: if caller and callee are in the same diff and signature/args don't match, must point out.
- If something looks suspicious but is actually fine based on this diff, put it in not_issues with reasoning; don't pad the issue list.

PR diff to review:
diff --git a/src/api/orders.py b/src/api/orders.py
@@ -12,6 +12,21 @@ from src.service.order_service import OrderService
 
+@app.route("/api/orders/search")
+def search_orders():
+    """Search orders by status and keyword, for ops backend"""
+    status = request.args.get("status", "paid")
+    kw = request.args.get("kw", "")
+    tenant = g.current_tenant_id
+    sql = (
+        f"SELECT id, uid, amount, status FROM orders "
+        f"WHERE tenant_id = {tenant} AND status = '{status}' "
+        f"AND title LIKE '%{kw}%' ORDER BY created_at DESC LIMIT 200"
+    )
+    rows = db.execute(sql).fetchall()
+    tmp1 = [dict(r) for r in rows]
+    return jsonify(tmp1)
+

diff --git a/src/service/order_service.py b/src/service/order_service.py
@@ -30,10 +30,25 @@ class OrderService:
 
-    def list_by_user(self, uid):
-        return db.query(Order).filter(Order.uid == uid).all()
+    def list_by_user(self, uid):
+        key = f"order:list:{uid}"
+        hit = self.cache.get(key)
+        if hit is not None:
+            return json.loads(hit)
+        rows = (db.query(Order)
+                  .filter(Order.uid == uid,
+                          Order.tenant_id == g.current_tenant_id)
+                  .all())
+        self.cache.set(key, json.dumps([r.to_dict() for r in rows]), ex=300)
+        return rows
+
+
+def build_report(month: int):
+    """Generate order report for a given month"""
+    if month < 1 or month > 12:
+        raise ValueError("month out of range")
+    return _aggregate(month)

diff --git a/src/api/report.py b/src/api/report.py
@@ -5,6 +5,11 @@ from src.service.order_service import build_report
 
+@app.route("/api/orders/report")
+def order_report():
+    month = request.args["month"]
+    data = build_report(month)
+    return jsonify(data)

diff --git a/src/service/payment.py b/src/service/payment.py
@@ -58,10 +58,16 @@ class PaymentService:
     def settle(self, order_id):
         order = self._load(order_id)
-        self.gateway.charge(order.uid, order.amount)
-        order.status = "paid"
-        db.commit()
+        try:
+            self.gateway.charge(order.uid, order.amount)
+        except Exception:
+            pass
+        order.status = "paid"
+        db.commit()
+        self.notifier.push(order.uid, "payment successful")

diff --git a/src/util/etag.py b/src/util/etag.py
@@ -0,0 +1,9 @@
+import hashlib
+
+
+def make_etag(payload: bytes) -> str:
+    """Compute an ETag for the response body, used only for negotiated caching, no credentials involved"""
+    return hashlib.md5(payload).hexdigest()

diff --git a/src/api/handler.py b/src/api/handler.py
@@ -33,8 +33,7 @@ def handle_request(req):
     ctx = build_context(req)
-    print("DEBUG ctx:", ctx)
-    resp = dispatch(ctx, timeout=cfg.timeout)
+    resp = dispatch(ctx, timeout=300)
     return resp

Key parameters:

# Pre-commit second-level screening: thinking off
{
  "model": "qwen3.8-flash",
  "messages": [{"role": "user", "content": "…git diff body…"}],
  "enable_thinking": false
}

# Final pass before merge: don't pass enable_thinking, use default thinking on

Under OpenAI-compatible mode, enable_thinking goes at the top level of the request body; flash defaults to thinking on.

{
  "verdict": "request_changes",
  "summary": "This PR has serious security vulnerabilities (SQL injection, missing auth) and logic errors (swallowed payment exception causing financial loss, type mismatch crash). Must fix SQL construction and payment status logic, and add input validation and cache invalidation before merge.",
  "issues": [
    {
      "file": "src/api/orders.py",
      "where": "search_orders",
      "severity": "blocking",
      "problem": "Directly uses f-string to concatenate SQL, without parameterized queries or escaping for `status` and `kw`.",
      "impact": "An attacker can execute arbitrary SQL via a crafted `kw` or `status` parameter, leading to database data leakage or tampering.",
      "fix": "Use ORM or parameterized queries.\n```python\n# Example: SQLAlchemy parameterized query\nfrom sqlalchemy import text\nsql = text(\"SELECT id, uid, amount, status FROM orders WHERE tenant_id = :tenant AND status = :status AND title LIKE :kw ORDER BY created_at DESC LIMIT 200\")\nrows = db.execute(sql, {\"tenant\": tenant, \"status\": status, \"kw\": f\"%{kw}%\"}).fetchall()\n```"
    },
    {
      "file": "src/service/payment.py",
      "where": "settle",
      "severity": "blocking",
      "problem": "Catches all exceptions from `gateway.charge` but only passes, then unconditionally sets order status to 'paid' and sends success notification.",
      "impact": "When gateway charge fails, the system still marks the order as paid and notifies the user, causing financial loss (delivering goods/services without receiving payment) and customer complaints.",
      "fix": "Remove try-except or only log then re-raise; ensure status is updated only on successful charge.\n```python\ndef settle(self, order_id):\n    order = self._load(order_id)\n    try:\n        self.gateway.charge(order.uid, order.amount)\n    except Exception as e:\n        # log\n        raise e\n    order.status = \"paid\"\n    db.commit()\n    self.notifier.push(order.uid, \"payment successful\")\n```"
    },
    {
      "file": "src/api/report.py",
      "where": "order_report",
      "severity": "blocking",
      "problem": "`request.args[\"month\"]` returns a string, passed directly to `build_report` which expects `int`, and no KeyError or ValueError handling.",
      "impact": "Missing month param throws KeyError (500); if present, `build_report` comparing `if month < 1` with string vs int throws TypeError (500).",
……

Notes

• Consumption: This playbook uses text generation (qwen3.8-flash). Thinking off: no reasoning cost; thinking on: output ~10× larger, mostly reasoning chain—cost estimation can't just look at the review report length.
• Measured result: Thinking-off read 6 files, verdict request_changes, findings 4 blocking + 1 suggestion + 1 nit, hit 5 of 7 test points—SQL injection (gave parameterized fix), except Exception: pass swallowing charge failure (wrote "goods delivered without payment" consequence), cross-file type mismatch (caller passes string, callee signature is int); decoy not false-positived (hashlib.md5 for ETag put in not_issues).
• Note: Thinking-off missed "cache key uses only uid without tenant_id, causing cross-tenant data bleed" (needs combining two changes to spot); switching to thinking-on hit all 7, flagged that one as blocking, but time and output volume rose significantly (this behavioral difference was consistent across an independent reproduction). Recommend two passes: thinking-off for pre-commit second-level screening, thinking-on for a final merge gate.
• Note: Thinking-off summary mentioned items not in the issues list; when integrating or citing, use issues as source of truth; both configs are assistive only, not a replacement for human review.
• Data boundary: Code in context means content goes to the server. Replace real secrets, internal addresses, and customer data with placeholders before submitting; keep code structure, the model can still spot issues.