pr-review

{
  "repo": "Levanto/Levanto-Aion",
  "review": {
    "summary": "This PR adds a per-repo project-context layer: a new DB table, tool, agent, and workflow, plus wiring through review-agent and existing workflows so creative agents see project-specific context. The implementation is thoughtful (UPSERT semantics, prompt-injection defenses with per-request nonces, graceful degradation on cache misses, URL.create for DSN safety, status-pending-before-ensure-context to keep PR authors informed). Findings are all minor — docstring/code drift on retry behaviour, unused `head_sha` argument in `refresh_needed`, serial GitHub fetches that could parallelise, and a missing belt-and-braces marker scrub. Confidence: high; no critical or major issues identified in the visible diff.",
    "verdict": "approved",
    "findings": [
      {
        "file": "tools/project_context/tool.py",
        "lens": "security",
        "line": 0,
        "finding": "The docstring rationale for using URL.create vs f-string interpolation is correct, but the resulting DSN is then passed through str(url.render_as_string(hide_password=False)) — note that any subsequent logging of the engine URL (e.g. by SQLAlchemy echo or exception traces) could leak the password; consider ensuring `echo=False` is the default (it appears to be) and avoiding logging the raw DSN.",
        "severity": "minor"
      },
      {
        "file": "tools/project_context/tool.py",
        "lens": "architecture",
        "line": 0,
        "finding": "`refresh_needed` now ignores the `head_sha` argument entirely (returns True only when no row / NULL sha). The parameter is retained for API compatibility but is unused — either drop it from the signature or document via a no-op comment, otherwise callers will reasonably assume per-SHA semantics that don't exist.",
        "severity": "minor"
      },
      {
        "file": "agents/project-context-agent/agent.py",
        "lens": "performance",
        "line": 0,
        "finding": "Doc/manifest fetching is performed serially with ~22 sequential GitHub API round trips on a cold-cache index (9 doc paths + 13 manifest paths). The comment acknowledges asyncio.gather would be 'nicer' — given this runs inline in the `ensure-context` state of pr-review (blocking the review gate), parallelising with a small semaphore would meaningfully cut cold-cache p99 latency.",
        "severity": "minor"
      },
      {
        "file": "agents/project-context-agent/agent.py",
        "lens": "architecture",
        "line": 0,
        "finding": "The docstring promises retry-with-stricter-framing on malformed JSON ('extract_json's bracket-walker salvages partial output, and the agent retries once with stricter \"JSON ONLY, NO PREAMBLE\" framing before failing'), but the implementation does not actually retry — it raises ValueError on the first JSON decode failure. Either implement the retry or correct the docstring.",
        "severity": "minor"
      },
      {
        "file": "agents/project-context-agent/agent.py",
        "lens": "operational",
        "line": 0,
        "finding": "`import json` is performed inside `_execute` (twice — once for parsing, once for size calculation). Move to module-level imports per project convention; the inline import adds no benefit and slightly obscures the dependency graph.",
        "severity": "minor"
      },
      {
        "file": "apps/orchestrator-api/app/api/routes/webhooks.py",
        "lens": "operational",
        "line": 0,
        "finding": "The default_branch fallback uses `'main'` when the repository payload lacks a `default_branch` field. For repos whose default branch is e.g. `master` or `develop`, a malformed/partial payload would silently fail to enqueue context refresh. This is low-impact (refresh would just be skipped) but worth a debug log when the field is absent vs present-but-non-matching.",
        "severity": "minor"
      },
      {
        "file": "agents/project-context-agent/agent.py",
        "lens": "security",
        "line": 0,
        "finding": "The UNTRUSTED-PROJECT-DATA marker scheme uses a per-request nonce (good), but unlike review-agent there is no defensive `_scrub` pass to neutralise any literal `<UNTRUSTED-PROJECT-DATA-{32hex}>`-looking substrings that might appear inside docs/manifests. The risk is low (32-hex nonces are not guessable), but the review-agent comments explicitly call out belt-and-braces scrubbing as a defense-in-depth measure; the project-context-agent skips that layer.",
        "severity": "minor"
      },
      {
        "file": "agents/project-context-agent/agent.py",
        "lens": "operational",
        "line": 0,
        "finding": "Unused imports: `ProjectContextReport`/`ProjectContextInput`/`ProjectContextOutput` import `Field` and use it, but the module also imports `ModelChoice` which is used only inside one call. More importantly, the `summary_size_bytes` is set on the success path but the early-return cache-hit path never sets it (defaults to 0) — this is fine but worth noting since the manifest.yaml documents `summary_size_kb` as an output field that the agent never actually populates with the kb-suffixed name.",
        "severity": "minor"
      }
    ]
  },
  "base_ref": "main",
  "head_sha": "9391c9eea70fdfe2dc4e84331abac4515620507a",
  "pr_number": 17,
  "post-comment": {
    "posted": {
      "posted": true,
      "pr_number": 17,
      "body_chars": 4722,
      "comment_id": 4541772315
    }
  },
  "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/c462d18e-6a0e-48c0-9e4b-49d5c7ce096a",
  "status-pending-review": {
    "posted": {
      "sha": "9391c9eea70fdfe2dc4e84331abac4515620507a",
      "state": "pending",
      "posted": true,
      "context": "aion/review",
      "target_url": "http://aion.levanto.in/runs/c462d18e-6a0e-48c0-9e4b-49d5c7ce096a"
    }
  },
  "status-success-review": {
    "posted": {
      "sha": "9391c9eea70fdfe2dc4e84331abac4515620507a",
      "state": "success",
      "posted": true,
      "context": "aion/review",
      "target_url": "http://aion.levanto.in/runs/c462d18e-6a0e-48c0-9e4b-49d5c7ce096a"
    }
  }
}