feat: Enhance review prompt with detailed instructions and placeholders for empty sections
This commit is contained in:
@@ -4,7 +4,6 @@ import base64
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
import re
|
||||
import shlex
|
||||
import subprocess
|
||||
import uuid
|
||||
@@ -12,14 +11,47 @@ from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
from gitea_codex_bot.config import Settings
|
||||
from gitea_codex_bot.services.gitea import GiteaClient
|
||||
from gitea_codex_bot.services.repo_config import RepoReviewConfig
|
||||
from gitea_codex_bot.services.reviewer import normalize_review_result, prepare_review_prompt
|
||||
from gitea_codex_bot.services.gitea import GiteaClient, PullRequestContext
|
||||
from gitea_codex_bot.services.repo_config import RepoReviewConfig, parse_repo_review_config_text
|
||||
from gitea_codex_bot.services.reviewer import normalize_review_result
|
||||
from gitea_codex_bot.types import ParsedCommand
|
||||
|
||||
CONTAINER_CODEX_HOME = "/root/.codex"
|
||||
REVIEW_OUTPUT_FILE = "/tmp/codex-review-result.json"
|
||||
REVIEW_SCHEMA_FILE = "/tmp/codex-review-schema.json"
|
||||
RESULT_START_MARKER = "__CODEX_REVIEW_RESULT_BEGIN__"
|
||||
RESULT_END_MARKER = "__CODEX_REVIEW_RESULT_END__"
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
REVIEW_RESULT_SCHEMA: dict[str, Any] = {
|
||||
"type": "object",
|
||||
"additionalProperties": True,
|
||||
"required": ["verdict", "confidence", "summary", "findings", "markdown_comment"],
|
||||
"properties": {
|
||||
"verdict": {"type": "string", "enum": ["correct", "has_issues"]},
|
||||
"confidence": {"type": "number"},
|
||||
"summary": {"type": "string"},
|
||||
"markdown_comment": {"type": "string"},
|
||||
"findings": {
|
||||
"type": "array",
|
||||
"items": {
|
||||
"type": "object",
|
||||
"additionalProperties": True,
|
||||
"required": ["severity", "file", "line_start", "line_end", "title", "body"],
|
||||
"properties": {
|
||||
"severity": {"type": "string", "enum": ["low", "medium", "high", "critical"]},
|
||||
"file": {"type": "string"},
|
||||
"line_start": {"type": "integer"},
|
||||
"line_end": {"type": "integer"},
|
||||
"title": {"type": "string"},
|
||||
"body": {"type": "string"},
|
||||
"suggestion": {"type": "string"},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
def run_review_ephemeral(
|
||||
settings: Settings,
|
||||
@@ -29,14 +61,27 @@ def run_review_ephemeral(
|
||||
command: ParsedCommand,
|
||||
) -> tuple[dict[str, Any], RepoReviewConfig]:
|
||||
gitea = GiteaClient(settings)
|
||||
prompt, _diff_context, repo_cfg = prepare_review_prompt(settings, gitea, repo, pr_number, command)
|
||||
pr = gitea.get_pull_request(repo, pr_number)
|
||||
repo_cfg = _load_repo_review_config_from_gitea(gitea, repo, pr.head_sha)
|
||||
_apply_repo_default_review_mode(command, repo_cfg)
|
||||
prompt = _build_review_instructions(command, repo_cfg)
|
||||
container_name = f"codex-review-{uuid.uuid4().hex[:12]}"
|
||||
extra_env: dict[str, str] = {}
|
||||
extra_env: dict[str, str] = {
|
||||
"GITEA_TOKEN": settings.gitea_token.get_secret_value(),
|
||||
"GITEA_GIT_USERNAME": settings.gitea_bot_username,
|
||||
}
|
||||
if settings.openai_api_key:
|
||||
extra_env["OPENAI_API_KEY"] = settings.openai_api_key.get_secret_value()
|
||||
if settings.openai_org_id:
|
||||
extra_env["OPENAI_ORG_ID"] = settings.openai_org_id
|
||||
if settings.openai_project_id:
|
||||
extra_env["OPENAI_PROJECT_ID"] = settings.openai_project_id
|
||||
if settings.codex_auth_mode == "chatgpt":
|
||||
extra_env["CODEX_AUTH_JSON_B64"] = _load_codex_auth_json_b64(settings)
|
||||
try:
|
||||
completed = _run_ephemeral_container(
|
||||
settings,
|
||||
pr=pr,
|
||||
container_name=container_name,
|
||||
prompt=prompt,
|
||||
extra_env=extra_env,
|
||||
@@ -46,14 +91,18 @@ def run_review_ephemeral(
|
||||
logger.info("Ephemeral runner does not support --reasoning-effort; retrying without it.")
|
||||
completed = _run_ephemeral_container(
|
||||
settings,
|
||||
pr=pr,
|
||||
container_name=container_name,
|
||||
prompt=prompt,
|
||||
extra_env=extra_env,
|
||||
include_reasoning_effort=False,
|
||||
)
|
||||
if completed.returncode != 0:
|
||||
compat_failure = _summarize_review_prompt_compat_failure(completed)
|
||||
if compat_failure:
|
||||
raise RuntimeError(compat_failure)
|
||||
raise RuntimeError(_format_runner_failure(completed))
|
||||
parsed = _parse_codex_exec_stdout(completed.stdout)
|
||||
parsed = _parse_review_result_from_stdout_artifact(completed.stdout)
|
||||
parsed["_meta"] = _extract_result_meta_from_codex_stdout(completed.stdout, settings)
|
||||
return normalize_review_result(parsed), repo_cfg
|
||||
except Exception as exc:
|
||||
@@ -64,12 +113,13 @@ def run_review_ephemeral(
|
||||
def _run_ephemeral_container(
|
||||
settings: Settings,
|
||||
*,
|
||||
pr: PullRequestContext,
|
||||
container_name: str,
|
||||
prompt: str,
|
||||
extra_env: dict[str, str],
|
||||
include_reasoning_effort: bool,
|
||||
) -> subprocess.CompletedProcess[str]:
|
||||
install_and_run = _build_install_and_run_command(settings, include_reasoning_effort=include_reasoning_effort)
|
||||
install_and_run = _build_install_and_run_command(settings, pr=pr, include_reasoning_effort=include_reasoning_effort)
|
||||
cmd = _build_docker_command(settings, container_name=container_name, install_and_run=install_and_run)
|
||||
return subprocess.run(
|
||||
cmd,
|
||||
@@ -82,8 +132,25 @@ def _run_ephemeral_container(
|
||||
)
|
||||
|
||||
|
||||
def _build_install_and_run_command(settings: Settings, *, include_reasoning_effort: bool = True) -> str:
|
||||
def _build_install_and_run_command(
|
||||
settings: Settings,
|
||||
*,
|
||||
pr: PullRequestContext,
|
||||
include_reasoning_effort: bool = True,
|
||||
) -> str:
|
||||
steps = ["set -euo pipefail"]
|
||||
if settings.codex_auth_mode != "chatgpt":
|
||||
steps.extend(
|
||||
[
|
||||
'if [ -z "${OPENAI_API_KEY:-}" ]; then echo "OPENAI_API_KEY missing in runner env" >&2; exit 8; fi',
|
||||
]
|
||||
)
|
||||
steps.extend(
|
||||
[
|
||||
'if [ -z "${GITEA_TOKEN:-}" ]; then echo "GITEA_TOKEN missing in runner env" >&2; exit 8; fi',
|
||||
'if [ -z "${GITEA_GIT_USERNAME:-}" ]; then echo "GITEA_GIT_USERNAME missing in runner env" >&2; exit 8; fi',
|
||||
]
|
||||
)
|
||||
if settings.codex_auth_mode == "chatgpt":
|
||||
steps.extend(
|
||||
[
|
||||
@@ -94,19 +161,50 @@ def _build_install_and_run_command(settings: Settings, *, include_reasoning_effo
|
||||
)
|
||||
steps.extend(
|
||||
[
|
||||
"apt-get update >/tmp/apt-update.log 2>&1 && apt-get install -y --no-install-recommends ca-certificates >/tmp/apt-install.log 2>&1 || { rc=$?; echo 'ca-certificates install failed'; tail -n 80 /tmp/apt-update.log || true; tail -n 80 /tmp/apt-install.log || true; exit $rc; }",
|
||||
"apt-get update >/tmp/apt-update.log 2>&1 && apt-get install -y --no-install-recommends ca-certificates git >/tmp/apt-install.log 2>&1 || { rc=$?; echo 'ca-certificates/git install failed'; tail -n 80 /tmp/apt-update.log || true; tail -n 80 /tmp/apt-install.log || true; exit $rc; }",
|
||||
"npm install -g @openai/codex >/tmp/codex-install.log 2>&1 || { rc=$?; echo 'codex install failed'; tail -n 200 /tmp/codex-install.log || true; exit $rc; }",
|
||||
]
|
||||
)
|
||||
schema_json = json.dumps(REVIEW_RESULT_SCHEMA, separators=(",", ":"))
|
||||
steps.extend(
|
||||
[
|
||||
f"cat > {REVIEW_SCHEMA_FILE} <<'JSON'\n{schema_json}\nJSON",
|
||||
'auth_b64="$(printf "%s" "${GITEA_GIT_USERNAME}:${GITEA_TOKEN}" | base64 | tr -d \'\\n\')"',
|
||||
f'git -c http.extraHeader="Authorization: Basic $auth_b64" clone --no-tags --depth 80 {shlex.quote(pr.clone_url)} /work/repo',
|
||||
"cd /work/repo",
|
||||
f'git -c http.extraHeader="Authorization: Basic $auth_b64" fetch --no-tags origin {shlex.quote(pr.base_ref)} {shlex.quote(pr.head_ref)}',
|
||||
f"git checkout --detach {shlex.quote(pr.head_sha)}",
|
||||
'resolved_head="$(git rev-parse HEAD)"',
|
||||
f'if [ "$resolved_head" != {shlex.quote(pr.head_sha)} ]; then echo "Checked out SHA mismatch: expected {pr.head_sha}, got $resolved_head" >&2; exit 9; fi',
|
||||
"unset GITEA_TOKEN auth_b64",
|
||||
"git config --global --unset-all http.extraHeader >/dev/null 2>&1 || true",
|
||||
]
|
||||
)
|
||||
model = settings.openai_review_model.strip()
|
||||
reasoning_effort = settings.openai_reasoning_effort.strip()
|
||||
codex_exec_parts = ["codex exec --skip-git-repo-check --json"]
|
||||
codex_exec_parts = [
|
||||
"codex exec review",
|
||||
f"--base {shlex.quote(pr.base_sha)}",
|
||||
"--json",
|
||||
"--output-schema",
|
||||
shlex.quote(REVIEW_SCHEMA_FILE),
|
||||
"-o",
|
||||
shlex.quote(REVIEW_OUTPUT_FILE),
|
||||
]
|
||||
if model:
|
||||
codex_exec_parts.append(f"-m {shlex.quote(model)}")
|
||||
if include_reasoning_effort and reasoning_effort:
|
||||
codex_exec_parts.append(f"--reasoning-effort {shlex.quote(reasoning_effort)}")
|
||||
steps.append(" ".join(codex_exec_parts))
|
||||
return "; ".join(steps)
|
||||
codex_exec_parts.append("-")
|
||||
steps.extend(
|
||||
[
|
||||
" ".join(codex_exec_parts),
|
||||
f'echo "{RESULT_START_MARKER}"',
|
||||
f"cat {shlex.quote(REVIEW_OUTPUT_FILE)}",
|
||||
f'echo "{RESULT_END_MARKER}"',
|
||||
]
|
||||
)
|
||||
return "\n".join(steps)
|
||||
|
||||
|
||||
def _needs_reasoning_effort_compat_retry(completed: subprocess.CompletedProcess[str]) -> bool:
|
||||
@@ -147,6 +245,14 @@ def _build_docker_command(settings: Settings, *, container_name: str, install_an
|
||||
"OPENAI_PROJECT_ID",
|
||||
]
|
||||
)
|
||||
cmd.extend(
|
||||
[
|
||||
"-e",
|
||||
"GITEA_TOKEN",
|
||||
"-e",
|
||||
"GITEA_GIT_USERNAME",
|
||||
]
|
||||
)
|
||||
cmd.extend([settings.review_runner_image, "bash", "-lc", install_and_run])
|
||||
return cmd
|
||||
|
||||
@@ -215,27 +321,67 @@ def ensure_workdir(path: str) -> Path:
|
||||
return target
|
||||
|
||||
|
||||
def _parse_codex_exec_stdout(stdout: str) -> dict[str, Any]:
|
||||
last_text: str | None = None
|
||||
for line in stdout.splitlines():
|
||||
line = line.strip()
|
||||
if not line:
|
||||
continue
|
||||
try:
|
||||
payload = json.loads(line)
|
||||
except json.JSONDecodeError:
|
||||
continue
|
||||
if isinstance(payload, dict) and {"verdict", "summary", "findings"}.issubset(payload.keys()):
|
||||
return payload
|
||||
extracted = _extract_text(payload)
|
||||
if extracted:
|
||||
last_text = extracted
|
||||
parsed = _parse_review_json_from_text(extracted)
|
||||
if parsed:
|
||||
return parsed
|
||||
if not last_text:
|
||||
raise RuntimeError("codex exec output did not include parseable review payload text")
|
||||
raise RuntimeError(f"codex exec output text did not contain review JSON; text_tail={_tail_text(last_text, 400)}")
|
||||
def _load_repo_review_config_from_gitea(gitea: GiteaClient, repo: str, head_sha: str) -> RepoReviewConfig:
|
||||
content = gitea.get_file_content(repo, ".codex-review.yml", ref=head_sha)
|
||||
if content is None:
|
||||
return RepoReviewConfig(configured=False)
|
||||
return parse_repo_review_config_text(content, configured=True)
|
||||
|
||||
|
||||
def _apply_repo_default_review_mode(command: ParsedCommand, repo_cfg: RepoReviewConfig) -> None:
|
||||
if command.name != "review" or command.mode_explicit:
|
||||
return
|
||||
configured_mode = repo_cfg.default_mode
|
||||
command.mode = configured_mode if configured_mode in {"summary", "security", "performance", "tests", "full"} else "summary"
|
||||
|
||||
|
||||
def _build_review_instructions(command: ParsedCommand, repo_cfg: RepoReviewConfig) -> str:
|
||||
focus = ", ".join(repo_cfg.focus) if repo_cfg.focus else "correctness, security, maintainability"
|
||||
ignore = ", ".join(repo_cfg.ignore) if repo_cfg.ignore else "(none)"
|
||||
lines = [
|
||||
"Review this pull request using local git data in this checkout only.",
|
||||
"Focus on issues introduced by this PR.",
|
||||
"Prioritize correctness, security, data loss, broken behavior, bad migrations, and missing tests.",
|
||||
"Avoid style-only nitpicks.",
|
||||
f"Requested mode: {command.mode}",
|
||||
f"Command: {command.raw}",
|
||||
f"Focus areas: {focus}",
|
||||
f"Ignore patterns: {ignore}",
|
||||
f"Repository include_tests setting: {repo_cfg.include_tests}",
|
||||
f"Full-content review requested: {command.full}",
|
||||
"Return strict JSON matching the provided output schema.",
|
||||
]
|
||||
return "\n".join(lines)
|
||||
|
||||
|
||||
def _parse_review_result_from_stdout_artifact(stdout: str) -> dict[str, Any]:
|
||||
start = stdout.find(RESULT_START_MARKER)
|
||||
end = stdout.find(RESULT_END_MARKER)
|
||||
if start == -1 or end == -1 or end <= start:
|
||||
raise RuntimeError("Runner output did not include final review artifact markers.")
|
||||
artifact = stdout[start + len(RESULT_START_MARKER) : end].strip()
|
||||
if not artifact:
|
||||
raise RuntimeError("Runner output contained empty final review artifact.")
|
||||
try:
|
||||
payload = json.loads(artifact)
|
||||
except json.JSONDecodeError as exc:
|
||||
raise RuntimeError(f"Final review artifact was not valid JSON: {exc}") from exc
|
||||
if not isinstance(payload, dict):
|
||||
raise RuntimeError(f"Final review artifact JSON must be an object, got {type(payload)!r}.")
|
||||
return payload
|
||||
|
||||
|
||||
def _summarize_review_prompt_compat_failure(completed: subprocess.CompletedProcess[str]) -> str | None:
|
||||
text = " ".join([(completed.stdout or "").strip(), (completed.stderr or "").strip()]).lower()
|
||||
has_prompt_conflict = "prompt" in text and (
|
||||
"cannot be used with" in text or "can't be used with" in text or "incompatible" in text
|
||||
)
|
||||
if "--base" not in text or not has_prompt_conflict:
|
||||
return None
|
||||
return (
|
||||
"Installed Codex CLI rejected `codex exec review --base ...` with custom instructions. "
|
||||
"This runner is configured to fail fast on that compatibility issue."
|
||||
)
|
||||
|
||||
|
||||
def _extract_result_meta_from_codex_stdout(stdout: str, settings: Settings) -> dict[str, Any]:
|
||||
@@ -297,49 +443,3 @@ def _find_first_dict_for_key(payload: Any, key: str) -> dict[str, Any] | None:
|
||||
if found:
|
||||
return found
|
||||
return None
|
||||
|
||||
|
||||
def _parse_review_json_from_text(text: str) -> dict[str, Any] | None:
|
||||
candidates: list[str] = [text.strip()]
|
||||
fenced = re.search(r"```(?:json)?\s*(\{.*\})\s*```", text, flags=re.DOTALL | re.IGNORECASE)
|
||||
if fenced:
|
||||
candidates.append(fenced.group(1).strip())
|
||||
start = text.find("{")
|
||||
end = text.rfind("}")
|
||||
if start != -1 and end != -1 and end > start:
|
||||
candidates.append(text[start : end + 1].strip())
|
||||
seen: set[str] = set()
|
||||
for candidate in candidates:
|
||||
if not candidate or candidate in seen:
|
||||
continue
|
||||
seen.add(candidate)
|
||||
try:
|
||||
payload = json.loads(candidate)
|
||||
except json.JSONDecodeError:
|
||||
continue
|
||||
if isinstance(payload, dict) and {"verdict", "summary", "findings"}.issubset(payload.keys()):
|
||||
return payload
|
||||
return None
|
||||
|
||||
|
||||
def _extract_text(payload: Any) -> str | None:
|
||||
if isinstance(payload, str):
|
||||
return payload
|
||||
if isinstance(payload, dict):
|
||||
for key in ("text", "message", "content", "output"):
|
||||
value = payload.get(key)
|
||||
text = _extract_text(value)
|
||||
if text:
|
||||
return text
|
||||
for value in payload.values():
|
||||
if not isinstance(value, (dict, list)):
|
||||
continue
|
||||
text = _extract_text(value)
|
||||
if text:
|
||||
return text
|
||||
if isinstance(payload, list):
|
||||
for item in payload:
|
||||
text = _extract_text(item)
|
||||
if text:
|
||||
return text
|
||||
return None
|
||||
|
||||
Reference in New Issue
Block a user