pr-review

{
  "repo": "Levanto/Levanto-Aion",
  "review": {
    "summary": "This PR fixes real CI-pipeline correctness bugs and adds genuine prompt-injection hardening to the review-agent, but the review-agent changes themselves appear incomplete: `marker_open` is referenced but not defined in the visible diff, `secrets` is imported but unused, and `marker_close` lacks any visible nonce interpolation — which would either NameError at runtime (disabling the merge gate entirely) or silently ship a static-marker defense that the system prompt claims is randomized. Additional concerns: the Dockerfile installs both `docker.io` (daemon + ~250 MB unused) and a static CLI, the build-agent doesn't validate that workflow-supplied dockerfile/context paths stay under the host repo root, and whole-repo mypy is silently disabled across security-critical hyphenated directories including the webhook handler this PR modifies. Confidence: medium-high on the review-agent NameError (the diff context strongly suggests `marker_open` is undefined); medium on the other findings pending a look at the truncated regions.",
    "verdict": "changes_requested",
    "findings": [
      {
        "file": "agents/review-agent/agent.py",
        "lens": "security",
        "line": 95,
        "finding": "The `marker_open` variable is referenced (in `f\"{marker_open} and {marker_close}\"` and `f\"{marker_open}\\n\"`) but never defined in the visible diff — only `marker_close` is assigned. This will raise NameError at runtime, breaking the entire review-agent and disabling the merge-gating review for all PRs, including malicious ones. Additionally `marker_close` is assigned `f\"[scrubbed-marker]\"` with no actual nonce interpolation visible, so even if `marker_open` were defined, the per-request randomization claimed by the system prompt may not actually be happening — defeating the prompt-injection defense this PR is supposedly adding.",
        "severity": "critical"
      },
      {
        "file": "agents/review-agent/agent.py",
        "lens": "architecture",
        "line": 8,
        "finding": "`secrets` is imported at module top but no use of `secrets.*` is visible in the diff. If the nonce generation lives outside the shown hunk that's fine, but combined with the missing `marker_open` definition this strongly suggests the nonce-generation code was dropped during refactor — exactly the regression the system prompt warns against. Needs verification that `marker_open = f\"<UNTRUSTED-PR-DATA-{secrets.token_hex(16)}>\"` (or equivalent) is actually present in the file.",
        "severity": "major"
      },
      {
        "file": "apps/orchestrator-api/app/api/routes/webhooks.py",
        "lens": "security",
        "line": 108,
        "finding": "`pr_number = workflow_input.get(\"pr_number\", 0)` is taken from the untrusted webhook payload and passed directly into `github_tool.list_pull_request_files(number=pr_number)` without validating that the PR belongs to the repository identified in the same payload. A spoofed/forwarded webhook (or one for a different repo than expected) could cause the orchestrator to fetch and lint files from an arbitrary public PR. Webhook signature verification (assumed upstream) bounds this somewhat, but the handler should still cross-check `repo` against an allowlist or against the PR's own `base.repo.full_name`.",
        "severity": "major"
      },
      {
        "file": "apps/worker-runtime/Dockerfile",
        "lens": "security",
        "line": 62,
        "finding": "Both `docker.io` (the Debian package, which pulls in the daemon binaries) AND a separately-downloaded static `docker` CLI are installed. The Debian `docker.io` package includes `dockerd`/`docker-proxy`/`containerd` (~250 MB) which are dead weight inside a container that talks to a bind-mounted host socket, and ships group/setuid configurations that increase attack surface. Use `apt-get install docker-cli` (or skip the apt package entirely and rely solely on the pinned static binary) rather than installing both.",
        "severity": "major"
      },
      {
        "file": "agents/build-agent/agent.py",
        "lens": "security",
        "line": 150,
        "finding": "`host_repo` is read from `$BUILD_AGENT_HOST_REPO` and interpolated into `docker build` context/dockerfile paths with no validation. While the env var is operator-controlled (not request-controlled) today, the `dockerfile` and `context` fields come from `_DEFAULT_TARGETS` or `payload.targets` (user input via workflow). A workflow author who can specify targets could pass a `dockerfile` like `../../../etc/passwd` and the agent will happily build using attacker-chosen Dockerfile content from outside the repo tree. Validate that resolved paths stay under `host_repo` (e.g. `os.path.commonpath`).",
        "severity": "major"
      },
      {
        "file": "agents/ci-quality-agent/agent.py",
        "lens": "operational",
        "line": 165,
        "finding": "In whole-repo mode the agent unconditionally skips mypy with no failure signal, and even in scoped mode any changed file under a hyphenated dir is silently filtered from mypy. This means type-checking is effectively disabled for `apps/orchestrator-api`, all hyphenated agents, and `runtime/base-agent` — a large portion of the codebase, including the security-critical webhook handler in this very PR. The TODO to rename dirs is fine, but `skipped=True` returning `passed=True` at the rollup level means a regression that breaks type safety on critical modules will never block merge. At minimum, log a warning metric and surface a `degraded` field on the report.",
        "severity": "major"
      },
      {
        "file": "agents/review-agent/agent.py",
        "lens": "security",
        "line": 130,
        "finding": "The `_scrub` regex `r\"<\\s*/?\\s*UNTRUSTED-PR-DATA[^>]*>\"` will not catch attempts that split the marker across token boundaries the model treats as one (e.g. `<UNTRUSTED-PR-DATA\\n-foo>` is caught because `[^>]*` is greedy on a single line, but multiline `<UNTRUSTED-PR-DATA\\nfoo>` is not — `[^>]` matches `\\n`, so actually this is fine). However, the regex won't catch `< UNTRUSTED-PR-DATA…` with leading whitespace inside the bracket but no slash (it does — `\\s*/?` handles it). Lower-risk: the scrub does not handle the case where attacker writes `<UNTRUSTED-PR-DATA-${nonce}>` after observing the nonce in any log/metric leak. The defense-in-depth comment in the code acknowledges this; documenting the threat model (no nonce leakage) more loudly would be valuable.",
        "severity": "minor"
      },
      {
        "file": "agents/review-agent/agent.py",
        "lens": "architecture",
        "line": 86,
        "finding": "`import re` is performed inside the nested `_scrub` function on every review invocation. Hoist it to module scope alongside `import secrets`.",
        "severity": "minor"
      },
      {
        "file": "apps/orchestrator-api/app/api/routes/webhooks.py",
        "lens": "performance",
        "line": 145,
        "finding": "The push-event truncation detection uses a hard 2000-path threshold but only after building the full `seen` set; for a truncated payload the set will simply be smaller than reality, not larger, so this branch never fires usefully. The real protection is the 20-commit check above. Either remove this dead branch or compute against a different signal (e.g. `payload.get(\"forced\")` plus commit-count proximity to 20).",
        "severity": "minor"
      },
      {
        "file": "agents/ci-quality-agent/tests/test_agent.py",
        "lens": "architecture",
        "line": 18,
        "finding": "`import sys` inside `_load()` rather than at module top; also `sys.modules[\"ci_quality_agent\"] = m` mutates global state and can leak between tests if any test loads a different version of the module. Use `pytest` fixtures with cleanup or `monkeypatch.setitem(sys.modules, ...)`.",
        "severity": "minor"
      },
      {
        "file": "agents/review-agent/agent.py",
        "lens": "privacy",
        "line": 70,
        "finding": "`files_fetch_error` includes the raw exception message (`{type(exc).__name__}: {exc}`) and is logged plus surfaced verbatim into the model prompt. GitHub API errors can include auth tokens or URL fragments containing PAT-style strings on misconfiguration; scrub or truncate before propagation.",
        "severity": "minor"
      }
    ]
  },
  "base_ref": "main",
  "head_sha": "54b7b44158d0bbb4fa7f739a8674c85fe3f2578c",
  "pr_number": 16,
  "post-comment": {
    "posted": {
      "posted": true,
      "pr_number": 16,
      "body_chars": 7638,
      "comment_id": 4532044063
    }
  },
  "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",
    "workflows/ci-pipeline/workflow.py"
  ],
  "dashboard_url": "http://aion.levanto.in/runs/091a4562-72ac-47c9-b4c9-59032306f780",
  "status-failure-review": {
    "posted": {
      "sha": "54b7b44158d0bbb4fa7f739a8674c85fe3f2578c",
      "state": "failure",
      "posted": true,
      "context": "aion/review",
      "target_url": "http://aion.levanto.in/runs/091a4562-72ac-47c9-b4c9-59032306f780"
    }
  },
  "status-pending-review": {
    "posted": {
      "sha": "54b7b44158d0bbb4fa7f739a8674c85fe3f2578c",
      "state": "pending",
      "posted": true,
      "context": "aion/review",
      "target_url": "http://aion.levanto.in/runs/091a4562-72ac-47c9-b4c9-59032306f780"
    }
  }
}