pr-review

{
  "repo": "Levanto/Levanto-Aion",
  "review": {
    "summary": "This PR fixes a broad set of real CI-pipeline bugs (whole-repo ruff on docs PRs, unwritable cache dirs, mypy crashing on hyphenated dirs, missing docker CLI in the worker image, host-vs-container path mismatch in build-agent, missing changed_files population in the webhook) and substantially hardens the review-agent against prompt-injection by wrapping attacker-controlled PR data in a per-request-nonce marker, scrubbing forged markers, and adding graceful degradation on GitHub API failures plus full-file context to reduce hallucinated findings. The system-prompt trust-boundary section is well-written and the new test coverage for the ci-quality-agent meaningfully locks in the regressions. I see no critical or major issues — only minor polish items around the apt+static docker CLI duplication, stacked type-ignore codes, asymmetric truncation thresholds in the webhook, and ensuring skipped-check semantics can't accidentally turn a report red. Confidence: high; I inspected the full-file contents for every change and the security-critical paths (HMAC verify, nonce generation via `secrets.token_hex(16)`, marker scrub regex matching exactly 32 hex chars) are sound.",
    "verdict": "approved",
    "findings": [
      {
        "file": "agents/build-agent/agent.py",
        "lens": "operational",
        "line": 132,
        "finding": "The skipped-build error message uses an f-string with a literal `$` prefix (`f\"skipped: ${_HOST_REPO_PATH_ENV} not set\"`), which produces the string `skipped: $BUILD_AGENT_HOST_REPO not set` — that's likely intentional to look shell-like, but the leading `$` is a common typo for `{}` interpolation and could mislead readers; consider dropping the `$` or using `{_HOST_REPO_PATH_ENV!s}` without `$` for clarity.",
        "severity": "minor"
      },
      {
        "file": "agents/build-agent/agent.py",
        "lens": "architecture",
        "line": 153,
        "finding": "The comment claims `.rstrip(\"/.\")` would over-strip and replaces it with `.removesuffix(\"/.\")`, but `removesuffix` only strips an exact `/.` suffix — if `context` is `.` then `host_repo/.` becomes `host_repo` (correct), yet if context is `\"\"` you'd get `host_repo/` with no trailing strip; the targets table never produces an empty context today, but a single-line note that the suffix-strip relies on the exact `/.` form would prevent future regressions.",
        "severity": "minor"
      },
      {
        "file": "agents/ci-quality-agent/agent.py",
        "lens": "architecture",
        "line": 86,
        "finding": "The non-.py-changed-files short-circuit returns three CheckResults with `output=\"skipped: …\"`, but the corresponding test `test_skips_cleanly_when_changed_files_has_no_py` asserts `'skipped' in c.output` — fine — yet the report-level `passed=True` combined with `skipped=True` per-check means downstream consumers must inspect `skipped` to avoid double-counting a docs PR as a real green; the new `skipped` field is the right fix but the rollup `passed = all(r.passed for r in results)` does not consider skipped explicitly, so a check that's both skipped=True and passed=False would silently turn the report red — worth a guard or assertion that skipped implies passed.",
        "severity": "minor"
      },
      {
        "file": "apps/orchestrator-api/app/api/routes/webhooks.py",
        "lens": "operational",
        "line": 158,
        "finding": "The 2000-file fallback uses `>= 2000` while the 20-commit truncation explicitly uses `>` after a comment arguing `>=` would misclassify; the asymmetric thresholds may be intentional (GitHub's documented file cap is approximate) but the inconsistency between the two truncation checks deserves a one-line comment to prevent a future reviewer 'fixing' one to match the other.",
        "severity": "minor"
      },
      {
        "file": "agents/review-agent/agent.py",
        "lens": "performance",
        "line": 95,
        "finding": "`asyncio.gather` over `_fetch_one` with a semaphore bounds concurrent fetches to 8, but spawns ALL tasks up front — for a PR touching thousands of files this materializes thousands of coroutine objects and queues them on the semaphore; consider also capping the total number of files for which content is fetched (e.g. first N by patch size) so a giant PR doesn't balloon worker memory before the budget renderer trims output.",
        "severity": "minor"
      },
      {
        "file": "apps/worker-runtime/Dockerfile",
        "lens": "security",
        "line": 79,
        "finding": "The Dockerfile installs both Debian's `docker.io` package (which pulls the daemon and a large dependency tree) AND a static docker CLI binary at `/usr/local/bin/docker`; this leaves dockerd / docker-proxy / docker-init present inside the worker image as unused attack surface. Since only the client is needed (the worker talks to the host daemon via the bind-mounted socket), drop `docker.io` from the apt install list and rely solely on the pinned static CLI.",
        "severity": "minor"
      },
      {
        "file": "tools/github/tool.py",
        "lens": "architecture",
        "line": 27,
        "finding": "Stacking `# type: ignore[import-not-found,import-untyped,unused-ignore]` to paper over mypy-version differences buries real future import breakages; prefer pinning a mypy version in pyproject and using a single specific ignore code, with a TODO referencing the runtime-rename cleanup.",
        "severity": "minor"
      }
    ]
  },
  "base_ref": "main",
  "head_sha": "9c0145f2435cca60861858be422781244ab79390",
  "pr_number": 16,
  "post-comment": {
    "posted": {
      "posted": true,
      "pr_number": 16,
      "body_chars": 5152,
      "comment_id": 4532638901
    }
  },
  "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/7f0c9745-1f3a-455c-ba99-078e41530662",
  "status-pending-review": {
    "posted": {
      "sha": "9c0145f2435cca60861858be422781244ab79390",
      "state": "pending",
      "posted": true,
      "context": "aion/review",
      "target_url": "http://aion.levanto.in/runs/7f0c9745-1f3a-455c-ba99-078e41530662"
    }
  },
  "status-success-review": {
    "posted": {
      "sha": "9c0145f2435cca60861858be422781244ab79390",
      "state": "success",
      "posted": true,
      "context": "aion/review",
      "target_url": "http://aion.levanto.in/runs/7f0c9745-1f3a-455c-ba99-078e41530662"
    }
  }
}