feat(review): ADDITIONAL_CONTEXT_URL — repo-provided static context cached per review
Long agent loops re-send the brief prefix on every step; cheap reusable knowledge (architecture summary, module map, conventions, glossary) belongs in a versioned file the maintainers control so the agent doesn't re-derive it from the source tree on every PR. Two wiring paths, merged (env first): * env var PRAGENT_ADDITIONAL_CONTEXT_URL — comma-separated, deployment-wide * .pr-review.json:additional_context_urls — list[str], read from the PR's base branch (same trust boundary as the rest of the file) Implementation: * _parse_additional_context_env splits/dedupes/trims. * _resolve_additional_context_urls(config) merges env (first) + config (then, skipping env-dupes); caps at 8. * fetch_additional_context(urls) fetches each URL with urllib (5s timeout, http/https only — file://, javascript:, ftp:// rejected defensively), caches by URL in a module-level dict for the pod lifetime, truncates per-URL to 4k chars + total to 16k chars, best-effort (network errors are logged and skipped — never aborts the review). * Result injected into build_user_prompt under "## Repo-provided context" between repo config and prior reviews. In the opencode engine it lands in .pragent/brief.md under its own section. The brief explicitly labels each block's CONTENT as untrusted (same as PR description) — section heading is trustworthy, body isn't. * parse_repo_config accepts the field, caps at 8 entries, drops non-strings and empty strings. Docs: pilot/README-webhook.md "Repo-provided static context" section — env var + JSON example + Nexus raw-hosted recipe. Tests: 14 new (208 total), covering env merging + dedup, scheme rejection, per-URL cap, total cap, caching by URL, brief injection. All mock urllib with a context-manager stand-in (no real network). Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Marcos
@@ -1260,3 +1260,201 @@ def test_compact_prior_reviews_drops_prose_keeps_bullets():
|
||||
def test_compact_prior_reviews_empty_and_none():
|
||||
assert ai_review.compact_prior_reviews([]) == []
|
||||
assert ai_review.compact_prior_reviews(None) == []
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# ADDITIONAL_CONTEXT_URL — env var + per-repo config
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_parse_repo_config_accepts_additional_context_urls():
|
||||
raw = json.dumps({
|
||||
"additional_context_urls": [
|
||||
"https://nexus.example.com/raw/context.md",
|
||||
" https://other.example/x.md ",
|
||||
123, # ignored (non-string)
|
||||
"", # ignored (empty after strip)
|
||||
]
|
||||
})
|
||||
cfg = parse_repo_config(raw)
|
||||
assert "additional_context_urls" in cfg
|
||||
# Non-strings and empty are stripped; whitespace trimmed.
|
||||
assert cfg["additional_context_urls"] == [
|
||||
"https://nexus.example.com/raw/context.md",
|
||||
"https://other.example/x.md",
|
||||
]
|
||||
|
||||
|
||||
def test_parse_repo_config_additional_context_urls_capped_at_8():
|
||||
urls = [f"https://x.example/{i}.md" for i in range(20)]
|
||||
cfg = parse_repo_config(json.dumps({"additional_context_urls": urls}))
|
||||
assert len(cfg["additional_context_urls"]) == 8
|
||||
|
||||
|
||||
def test_parse_repo_config_additional_context_urls_absent_when_missing():
|
||||
assert "additional_context_urls" not in parse_repo_config("{}")
|
||||
|
||||
|
||||
def test_resolve_additional_context_urls_env_wins_and_dedupes(monkeypatch):
|
||||
monkeypatch.setenv(
|
||||
"PRAGENT_ADDITIONAL_CONTEXT_URL",
|
||||
"https://env.example/a.md, https://env.example/b.md",
|
||||
)
|
||||
cfg = {"additional_context_urls": [
|
||||
"https://env.example/a.md", # dup with env -> dropped from cfg list
|
||||
"https://cfg.example/d.md",
|
||||
]}
|
||||
urls = ai_review._resolve_additional_context_urls(cfg)
|
||||
# Env comes first, in declared order; cfg entries that duplicate env are skipped.
|
||||
assert urls == [
|
||||
"https://env.example/a.md",
|
||||
"https://env.example/b.md",
|
||||
"https://cfg.example/d.md",
|
||||
]
|
||||
|
||||
|
||||
def test_resolve_additional_context_urls_no_env_no_config():
|
||||
import os as _os
|
||||
_os.environ.pop("PRAGENT_ADDITIONAL_CONTEXT_URL", None)
|
||||
assert ai_review._resolve_additional_context_urls(None) == []
|
||||
assert ai_review._resolve_additional_context_urls({}) == []
|
||||
|
||||
|
||||
def test_resolve_additional_context_urls_total_cap_is_8(monkeypatch):
|
||||
monkeypatch.setenv(
|
||||
"PRAGENT_ADDITIONAL_CONTEXT_URL",
|
||||
",".join(f"https://e.example/{i}" for i in range(20)),
|
||||
)
|
||||
urls = ai_review._resolve_additional_context_urls({
|
||||
"additional_context_urls": [f"https://c.example/{i}" for i in range(20)]
|
||||
})
|
||||
assert len(urls) == 8
|
||||
|
||||
|
||||
class _FakeResp:
|
||||
"""Minimal stand-in for urllib's HTTP response: context manager + .read(N)."""
|
||||
|
||||
def __init__(self, body: bytes):
|
||||
import io as _io
|
||||
self._buf = _io.BytesIO(body)
|
||||
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, *a):
|
||||
return False
|
||||
|
||||
def read(self, n=-1):
|
||||
return self._buf.read(n)
|
||||
|
||||
|
||||
def _patch_urlopen(body_for_url):
|
||||
"""Replace ai_review.urllib.request.urlopen with a fake that returns the
|
||||
configured body for each URL. `body_for_url: dict[str, bytes]`. Records
|
||||
every URL it sees in `calls` on the closure."""
|
||||
calls: list[str] = []
|
||||
|
||||
def fake(req, *args, **kwargs):
|
||||
url = req.full_url if hasattr(req, "full_url") else str(req)
|
||||
calls.append(url)
|
||||
return _FakeResp(body_for_url.get(url, b""))
|
||||
|
||||
import ai_review as _ar
|
||||
orig = _ar.urllib.request.urlopen
|
||||
_ar.urllib.request.urlopen = fake
|
||||
|
||||
def restore():
|
||||
_ar.urllib.request.urlopen = orig
|
||||
|
||||
return calls, restore
|
||||
|
||||
|
||||
def test_fetch_additional_context_joins_blocks():
|
||||
ai_review._ADDITIONAL_CONTEXT_CACHE.clear()
|
||||
calls, restore = _patch_urlopen({
|
||||
"https://a/x.md": b"alpha body",
|
||||
"https://b/y.md": b"beta body",
|
||||
})
|
||||
try:
|
||||
out = ai_review.fetch_additional_context(["https://a/x.md", "https://b/y.md"])
|
||||
finally:
|
||||
restore()
|
||||
|
||||
assert "alpha body" in out and "beta body" in out
|
||||
assert calls == ["https://a/x.md", "https://b/y.md"]
|
||||
|
||||
|
||||
def test_fetch_additional_context_caches_by_url():
|
||||
ai_review._ADDITIONAL_CONTEXT_CACHE.clear()
|
||||
calls, restore = _patch_urlopen({"https://a/x.md": b"cached body"})
|
||||
try:
|
||||
ai_review.fetch_additional_context(["https://a/x.md"])
|
||||
ai_review.fetch_additional_context(["https://a/x.md", "https://a/x.md"])
|
||||
finally:
|
||||
restore()
|
||||
|
||||
# Second call hits cache; only one network call despite 3 references.
|
||||
assert calls == ["https://a/x.md"]
|
||||
|
||||
|
||||
def test_fetch_additional_context_rejects_non_http_schemes():
|
||||
ai_review._ADDITIONAL_CONTEXT_CACHE.clear()
|
||||
out = ai_review.fetch_additional_context([
|
||||
"file:///etc/passwd",
|
||||
"javascript:alert(1)",
|
||||
"ftp://x/y",
|
||||
])
|
||||
# All rejected at scheme check, no network calls.
|
||||
assert out == ""
|
||||
|
||||
|
||||
def test_fetch_additional_context_truncates_per_url():
|
||||
ai_review._ADDITIONAL_CONTEXT_CACHE.clear()
|
||||
cap = ai_review._ADDITIONAL_CONTEXT_MAX_PER_URL_CHARS
|
||||
_, restore = _patch_urlopen({"https://a/big.md": b"X" * (cap + 500)})
|
||||
try:
|
||||
out = ai_review.fetch_additional_context(["https://a/big.md"])
|
||||
finally:
|
||||
restore()
|
||||
|
||||
assert "…[truncated]" in out
|
||||
# The fetched body is bounded to `cap` chars (the marker + the URL
|
||||
# header are appended on top by the joiner, so we count just X's).
|
||||
assert out.count("X") == cap
|
||||
|
||||
|
||||
def test_fetch_additional_context_caps_total_chars():
|
||||
ai_review._ADDITIONAL_CONTEXT_CACHE.clear()
|
||||
cap_total = ai_review._ADDITIONAL_CONTEXT_MAX_TOTAL_CHARS
|
||||
# Each block: "### https://a/N.md\n\n" + 3990 X + "\nEND" ≈ 4017 chars.
|
||||
big = (b"X" * 3990) + b"\nEND"
|
||||
urls = [f"https://a/{i}.md" for i in range(8)]
|
||||
_, restore = _patch_urlopen({u: big for u in urls})
|
||||
try:
|
||||
out = ai_review.fetch_additional_context(urls)
|
||||
finally:
|
||||
restore()
|
||||
|
||||
# Total is bounded by the cap plus the truncation marker (if the last
|
||||
# block was cut mid-flight).
|
||||
assert len(out) <= cap_total + 20, len(out)
|
||||
|
||||
|
||||
def test_fetch_additional_context_empty_returns_empty():
|
||||
assert ai_review.fetch_additional_context([]) == ""
|
||||
|
||||
|
||||
def test_build_user_prompt_injects_additional_context():
|
||||
prompt = build_user_prompt(
|
||||
"T", "B", "diff", config=None, prior_reviews=None,
|
||||
additional_context="### https://a/x.md\n\nalpha body",
|
||||
)
|
||||
assert "## Repo-provided context" in prompt
|
||||
assert "alpha body" in prompt
|
||||
# URL header preserved so the agent knows which block is which.
|
||||
assert "https://a/x.md" in prompt
|
||||
|
||||
|
||||
def test_build_user_prompt_skips_additional_context_when_empty():
|
||||
prompt = build_user_prompt("T", "B", "diff")
|
||||
assert "## Repo-provided context" not in prompt
|
||||
|
||||
Reference in New Issue
Block a user