pr-review

{
  "repo": "Levanto/Levanto-Aion",
  "review": {
    "summary": "PR claims to harden the review-agent against prompt injection with a per-request 128-bit nonce, but the diff as shown defines both markers as the static literal `[scrubbed-marker]` with no nonce interpolation — defeating the stated defense and arguably regressing security relative to the (also-broken) previous version. Additionally, the worker Dockerfile pins a single x86_64 SHA256 against a `uname -m`-based URL that will break (or silently mismatch) on arm64 builds. Both blocking. Confidence is moderate: some review-agent source appears to have been pre-scrubbed by the renderer (the literal `[scrubbed-marker]` may be a scrub artifact rather than what's actually in source), so the nonce finding might be a false positive — but if the rendered source is faithful, the prompt-injection claim in the PR title is not delivered. The remaining changes (webhook changed_files population, ci-quality scoping, host-path build fix, QUICKSTART, sync-credentials logging, isort config) look correct and well-reasoned.",
    "verdict": "changes_requested",
    "findings": [
      {
        "file": "agents/review-agent/agent.py",
        "lens": "security",
        "line": 145,
        "finding": "The marker_open and marker_close variables are assigned to the literal string `[scrubbed-marker]` (an f-string with no nonce interpolation visible in the diff), meaning both opening and closing markers in the user prompt are the same static literal — completely defeating the per-request nonce defense the comment and system prompt advertise. An attacker can trivially include `[scrubbed-marker]` in their PR body to escape the data region. The _scrub() regex only targets `<...UNTRUSTED-PR-DATA...>` strings, not the literal `[scrubbed-marker]` text actually used as the marker, so attacker content is not scrubbed of marker collisions either.",
        "severity": "major"
      },
      {
        "file": "agents/review-agent/agent.py",
        "lens": "architecture",
        "line": 160,
        "finding": "The marker construction code in the diff is syntactically incomplete/broken: an unterminated string literal (`Belt-and-braces: we ALSO defensively replace any literal\\n  `[scrubbed-marker]\"`) appears followed by a `marker_close = f\"[scrubbed-marker]\"` assignment with no corresponding marker_open assignment visible. The full-file rendering shows the same scrubbed literal, suggesting the real source uses placeholders that were stripped — but as-rendered the module would not parse. Reviewer cannot fully verify (content may be scrubbed by the renderer), so confidence is reduced, but if the literal `[scrubbed-marker]` truly appears in source as shown, this is a syntax error blocking import.",
        "severity": "major"
      },
      {
        "file": "apps/worker-runtime/Dockerfile",
        "lens": "security",
        "line": 70,
        "finding": "The pinned DOCKER_CLI_SHA256 is hardcoded for x86_64, but the curl URL uses `$(uname -m)` to select the architecture-specific tarball. On an arm64 build host (e.g. Apple Silicon CI, Graviton), the script will download the aarch64 tarball and verify it against the x86_64 SHA256 — the sha256sum check will fail and the build will break, OR worse, if someone later updates only one arch's SHA the wrong-arch path silently succeeds against a mismatched binary. Either pin per-arch SHAs with a conditional, or hardcode the architecture in the URL to match the pinned hash.",
        "severity": "major"
      },
      {
        "file": "apps/orchestrator-api/app/api/routes/webhooks.py",
        "lens": "operational",
        "line": 145,
        "finding": "The push-event truncation detection uses `len(commits) >= 20` as a heuristic, but GitHub's documented cap is exactly 20 commits per push payload — a legitimate push of exactly 20 commits (no truncation) will be misclassified as possibly-truncated and fall back to whole-repo mode. Use `> 20` or check the `forced`/`compare` fields, or accept this is a precision-vs-correctness trade and document it clearly in the log.",
        "severity": "minor"
      },
      {
        "file": "agents/build-agent/agent.py",
        "lens": "operational",
        "line": 155,
        "finding": "`removesuffix(\"/.\")` only strips a single trailing `/.` so `host_repo/apps/dashboard` (context without trailing /.) is fine, but `host_repo/.` collapses to `host_repo` correctly. However the build context `\".\"` produces `f\"{host_repo}/.\"` which becomes `host_repo` after removesuffix — fine. The comment claims `.rstrip(\"/.\")` would over-strip but the diff still uses removesuffix correctly; this is fine functionally. Lower severity: the error message uses a literal `$` (`f\"skipped: ${_HOST_REPO_PATH_ENV} not set\"`) which renders as `$BUILD_AGENT_HOST_REPO` — readable but slightly misleading shell-syntax suggestion.",
        "severity": "minor"
      },
      {
        "file": "agents/ci-quality-agent/agent.py",
        "lens": "operational",
        "line": 95,
        "finding": "When `changed_files` contains only non-.py files, the agent returns `report.passed=True` with all checks marked passed=True/skipped=True. This is intentional per the comments, but a docs-only PR that happens to also modify a config file (e.g., pyproject.toml) — which is non-.py — would silently skip all linting. Consider extending the skip predicate to also short-circuit when py_paths is empty but logging the set of unmatched extensions for visibility.",
        "severity": "minor"
      },
      {
        "file": "agents/review-agent/agent.py",
        "lens": "performance",
        "line": 200,
        "finding": "`get_file_contents` is fetched in parallel via asyncio.gather for every changed file with no concurrency cap. A PR touching 200 files will issue 200 concurrent GitHub API requests, which can trip secondary rate limits (GitHub returns 403 with `Retry-After`). Bound with a semaphore (e.g., 8-10 concurrent) to stay within GitHub's abuse-detection thresholds.",
        "severity": "minor"
      },
      {
        "file": "tools/github/tool.py",
        "lens": "security",
        "line": 230,
        "finding": "`get_file_contents` uses `path` (caller-supplied) directly in the URL `/repos/{o}/{r}/contents/{path}` without URL-encoding. A path containing `..`, `?`, `#`, or other special characters could alter the request semantics. Filenames from `list_pull_request_files` are GitHub-controlled so attacker risk is low, but defensive URL-encoding via httpx params or urllib.parse.quote is cheap insurance.",
        "severity": "minor"
      }
    ]
  },
  "base_ref": "main",
  "head_sha": "dce892f787e9d70f1e924d95abc1e19a2a34b48d",
  "pr_number": 16,
  "post-comment": {
    "posted": {
      "posted": true,
      "pr_number": 16,
      "body_chars": 6137,
      "comment_id": 4532591440
    }
  },
  "changed_files": [
    "agents/build-agent/agent.py",
    "agents/ci-quality-agent/agent.py",
    "agents/ci-quality-agent/tests/test_agent.py",
    "agents/review-agent/agent.py",
    "agents/review-agent/prompts/system.md",
    "apps/orchestrator-api/app/api/routes/webhooks.py",
    "apps/worker-runtime/Dockerfile",
    "docs/MODEL_ROUTING.md",
    "docs/QUICKSTART.md",
    "docs/README.md",
    "infra/scripts/sync-claude-credentials.sh",
    "pyproject.toml",
    "tools/github/tool.py",
    "workflows/ci-pipeline/workflow.py"
  ],
  "dashboard_url": "http://aion.levanto.in/runs/d52f703d-4b6e-488c-b6a1-cc3b9b042d37",
  "status-failure-review": {
    "posted": {
      "sha": "dce892f787e9d70f1e924d95abc1e19a2a34b48d",
      "state": "failure",
      "posted": true,
      "context": "aion/review",
      "target_url": "http://aion.levanto.in/runs/d52f703d-4b6e-488c-b6a1-cc3b9b042d37"
    }
  },
  "status-pending-review": {
    "posted": {
      "sha": "dce892f787e9d70f1e924d95abc1e19a2a34b48d",
      "state": "pending",
      "posted": true,
      "context": "aion/review",
      "target_url": "http://aion.levanto.in/runs/d52f703d-4b6e-488c-b6a1-cc3b9b042d37"
    }
  }
}