pr-review

{
  "repo": "Levanto/Levanto-Aion",
  "review": {
    "summary": "This PR adds a per-repo project-context indexing layer (DB table, tool, agent, workflow, review-agent integration). The design is coherent and well-documented with strong prompt-injection awareness for the indexer's input. Blocking issues: (1) DSN string interpolation in the project_context tool is vulnerable to credential-character corruption/injection — use SQLAlchemy URL.create; (2) the refresh_needed cache key doesn't account for branch context and can yield false hits/misses; (3) ensure-context now blocks the pending commit-status by up to ~60s on cold cache, causing observability regressions for PR authors. Several minor architectural and operational concerns also noted around the project-context-derived data flowing into the system prompt outside trust-boundary markers. Confidence: high on the blocking findings, medium on the minor ones since I couldn't see the full state_machine engine semantics or the workers' tool-registry construction.",
    "verdict": "changes_requested",
    "findings": [
      {
        "file": "tools/project_context/tool.py",
        "lens": "security",
        "line": 0,
        "finding": "DSN is constructed via f-string interpolation of AION_DB_PASSWORD/user/host without URL-encoding; a password containing '@', ':', '/', '#' or '?' will produce a malformed DSN or, worse, allow a host-component injection (e.g. password 'foo@evil.host/' would point asyncpg at attacker-controlled host). Use sqlalchemy.engine.URL.create() or urllib.parse.quote for each component.",
        "severity": "major"
      },
      {
        "file": "agents/project-context-agent/agent.py",
        "lens": "architecture",
        "line": 0,
        "finding": "ensure-mode cache short-circuit calls store.refresh_needed(repo, head_sha) which returns False when last_indexed_sha != head_sha is false, but the existing row's SHA may belong to a DIFFERENT branch than the workflow's head_sha. refresh_needed compares against whatever last_indexed_sha is stored regardless of branch; a feature-branch ensure-context that happens to match the cached default-branch SHA would short-circuit incorrectly, and the inverse (cached default-branch SHA != feature head_sha) forces a wasteful re-index on every PR. The cache key should incorporate the SHA, not compare against an unrelated indexed SHA.",
        "severity": "major"
      },
      {
        "file": "workflows/pr-review/workflow.py",
        "lens": "operational",
        "line": 0,
        "finding": "ensure-context is now the initial state and runs an opus-4-7 indexing call (~30-60s per the PR description) BEFORE status-pending-review fires. This means the GitHub commit-status 'aion/review pending' check is delayed by up to a minute on cold-cache PRs — operators and PR authors see no signal that review is queued during indexing, and any indexing failure that takes longer than expected leaves the PR in an indeterminate state. Consider posting the pending status FIRST, then ensure-context, then review.",
        "severity": "major"
      },
      {
        "file": "agents/project-context-agent/agent.py",
        "lens": "operational",
        "line": 0,
        "finding": "The docstring claims a retry-once-on-malformed-JSON behavior ('retries once with stricter JSON ONLY framing before failing') but the actual code raises ValueError immediately on JSONDecodeError or missing JSON. Either implement the retry or update the docstring to reflect actual behavior.",
        "severity": "minor"
      },
      {
        "file": "agents/project-context-agent/agent.py",
        "lens": "architecture",
        "line": 0,
        "finding": "Sequential awaits for ~22 file fetches (9 docs + 13 manifests) on the cold path serially round-trip GitHub — easily 5-10s of avoidable latency on the path that already blocks the PR review. The comment acknowledges asyncio.gather would be nicer; given this runs on the hot path of pr-review and feature-to-prod, the parallelization is worth doing now.",
        "severity": "minor"
      },
      {
        "file": "tools/project_context/tool.py",
        "lens": "operational",
        "line": 0,
        "finding": "Each worker replica builds its own AsyncEngine with pool_size=5 + max_overflow=5 (10 conns/replica). The docstring's math ('3 replicas x 5 = 15') ignores max_overflow and undercounts by 2x. With multiple tools each owning engines and several worker replicas, cumulative connection use can exceed the Postgres default max_connections=100 under load. Consider sharing an engine via the platform DB dep or lowering max_overflow.",
        "severity": "minor"
      },
      {
        "file": "agents/review-agent/agent.py",
        "lens": "architecture",
        "line": 0,
        "finding": "_render_system_prompt builds the context block from a dict that flows from untrusted README/manifest content (model-summarized but still attacker-influenced). The block is prepended to the SYSTEM prompt, outside the UNTRUSTED-PR-DATA markers. An attacker who can craft a README that survives the indexer could plant strings in name/one_liner/conventions values that the model treats as system-level instructions on later reviews. Consider rendering the project_context block inside its own per-request nonced UNTRUSTED-PROJECT-CONTEXT markers, or at minimum scrub the field values for marker-looking strings before interpolating.",
        "severity": "minor"
      },
      {
        "file": "workflows/feature-to-prod/workflow.py",
        "lens": "operational",
        "line": 0,
        "finding": "The ensure-context payload uses '${head_sha}' but feature-to-prod is described as 'often invoked against a branch name rather than a commit' — when head_sha is empty/unset, project_context.refresh_needed returns False (per the tool's `if not head_sha: return False` branch), so a stale cache hit is silently accepted. The workflow should either require head_sha or fall back to '${branch}' explicitly in the template.",
        "severity": "minor"
      }
    ]
  },
  "base_ref": "main",
  "head_sha": "f4d491ea0f7fb352e085990434ebfe8c0cfaac16",
  "pr_number": 17,
  "post-comment": {
    "posted": {
      "posted": true,
      "pr_number": 17,
      "body_chars": 5573,
      "comment_id": 4534431974
    }
  },
  "changed_files": [
    "agents/project-context-agent/agent.py",
    "agents/project-context-agent/manifest.yaml",
    "agents/project-context-agent/prompts/system.md",
    "agents/project-context-agent/tests/__init__.py",
    "agents/project-context-agent/tests/test_agent.py",
    "agents/review-agent/agent.py",
    "agents/review-agent/tests/test_agent.py",
    "apps/orchestrator-api/app/api/routes/webhooks.py",
    "apps/orchestrator-api/app/models/__init__.py",
    "apps/orchestrator-api/app/models/project_context.py",
    "apps/orchestrator-api/migrations/versions/0002_project_contexts.py",
    "tools/project_context/manifest.yaml",
    "tools/project_context/tool.py",
    "workflows/feature-to-prod/workflow.py",
    "workflows/pr-review/workflow.py",
    "workflows/project-context-refresh/manifest.yaml",
    "workflows/project-context-refresh/workflow.py"
  ],
  "dashboard_url": "http://aion.levanto.in/runs/09b815dc-b2d7-436d-8535-d41b3af41df1",
  "status-failure-review": {
    "posted": {
      "sha": "f4d491ea0f7fb352e085990434ebfe8c0cfaac16",
      "state": "failure",
      "posted": true,
      "context": "aion/review",
      "target_url": "http://aion.levanto.in/runs/09b815dc-b2d7-436d-8535-d41b3af41df1"
    }
  },
  "status-pending-review": {
    "posted": {
      "sha": "f4d491ea0f7fb352e085990434ebfe8c0cfaac16",
      "state": "pending",
      "posted": true,
      "context": "aion/review",
      "target_url": "http://aion.levanto.in/runs/09b815dc-b2d7-436d-8535-d41b3af41df1"
    }
  }
}