Skip to content

Commit dab7c31

Browse files
committed
examples: route every input mode through the resolved sandbox runtime
The container runtime silently downgraded file-list and worktree inputs to the in-process path: only --diff-file and --fixture were wired to run_review_container, while --files and --repo-path called the sync run_review with runtime="container", which had no container branch and fell through to in-process. Extract the input materialization into a shared _resolve_input helper used by both entry points, extend run_review_container to accept all three input modes, and route every container request through it in the CLI. Sync run_review now rejects the container runtime loudly instead of falling back. Also correct two stale docstring and README references to a fixture that no longer exists. Updates #92 RELEASE NOTES: NONE
1 parent 14c5bbc commit dab7c31

5 files changed

Lines changed: 72 additions & 30 deletions

File tree

examples/skills_code_review_agent/README.zh_CN.md

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,17 +9,25 @@
99
```bash
1010
pip install -r requirements.txt
1111

12-
# 评审内置样本(dry-run,确定性,无需模型):
13-
python run_review.py --fixture 0001_insecure.diff --out-dir /tmp/cr
12+
# 评审内置样本(无需模型)。默认运行时是沙箱
13+
# (auto → 有 Docker 走容器,否则本地子进程沙箱):
14+
python run_review.py --fixture security.diff --out-dir /tmp/cr
1415

15-
# 评审你自己的 diff 或工作区
16+
# 评审你自己的 diff、工作区,或指定文件列表
1617
python run_review.py --diff-file my.diff
1718
python run_review.py --repo-path /path/to/repo --no-db
19+
python run_review.py --files pipeline/engine.py,pipeline/scanners.py
1820

1921
# 在带标注的样本上打分自测(检出率 / 误报率):
2022
python selftest.py
23+
24+
# 走 LlmAgent + fake 模型(无需 API key):
25+
python run_agent.py --fixture security.diff --dry-run
2126
```
2227

28+
样本报告见 [`sample_output/`](./sample_output/);规则清单见
29+
[`../../skills/code-review/docs/RULES.md`](../../skills/code-review/docs/RULES.md),设计说明见 [DESIGN.md](./DESIGN.md)
30+
2331
## 工作原理
2432

2533
findings 来自**确定性静态扫描器**而非 LLM,因此结果可复现、验收阈值可调:

examples/skills_code_review_agent/pipeline/engine.py

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -99,20 +99,10 @@ def run_review(
9999
# fast-path that must be requested explicitly.
100100
if runtime == "auto":
101101
runtime = "local"
102+
elif runtime == "container":
103+
raise ValueError("container runtime is async — call run_review_container() instead of run_review()")
102104

103-
if diff_text is not None:
104-
summary, scan_dir = _materialize(diff_text)
105-
source_type, source_ref = "diff_file", "<diff>"
106-
elif files is not None:
107-
summary = diff_parser.parse_file_list(files, repo_root)
108-
scan_dir = _materialize_files(files, repo_root)
109-
source_type, source_ref = "file_list", ",".join(files)[:200]
110-
elif repo_path is not None:
111-
summary = diff_parser.parse_git_worktree(repo_path)
112-
scan_dir = repo_path
113-
source_type, source_ref = "repo_path", repo_path
114-
else:
115-
raise ValueError("run_review requires diff_text, files, or repo_path")
105+
summary, scan_dir, source_type, source_ref = _resolve_input(diff_text, files, repo_path, repo_root)
116106

117107
sandbox_runs: list = []
118108
if runtime == "local":
@@ -185,18 +175,42 @@ def _assemble(task_id, summary, raw, sandbox_runs, source_type, source_ref, star
185175
monitoring=monitoring)
186176

187177

178+
def _resolve_input(diff_text: Optional[str], files: Optional[list[str]], repo_path: Optional[str],
179+
repo_root: str) -> tuple[DiffSummary, str, str, str]:
180+
"""Materialize any of the three input modes into (summary, scan_dir, source_type, source_ref).
181+
182+
Shared by run_review and run_review_container so every input mode reaches the same sandbox path.
183+
"""
184+
if diff_text is not None:
185+
summary, scan_dir = _materialize(diff_text)
186+
return summary, scan_dir, "diff_file", "<diff>"
187+
if files is not None:
188+
return diff_parser.parse_file_list(files, repo_root), _materialize_files(files, repo_root), \
189+
"file_list", ",".join(files)[:200]
190+
if repo_path is not None:
191+
return diff_parser.parse_git_worktree(repo_path), repo_path, "repo_path", repo_path
192+
raise ValueError("a review requires diff_text, files, or repo_path")
193+
194+
188195
async def run_review_container(
189196
*,
190197
task_id: Optional[str] = None,
191-
diff_text: str,
198+
diff_text: Optional[str] = None,
199+
files: Optional[list[str]] = None,
200+
repo_path: Optional[str] = None,
201+
repo_root: str = ".",
192202
sandbox_timeout: float | None = None,
193203
max_output_bytes: int | None = None,
194204
) -> ReviewResult:
195-
"""Run a review with scanners inside a Container workspace (production isolation; needs Docker)."""
205+
"""Run a review with scanners inside a Container workspace (production isolation; needs Docker).
206+
207+
Accepts the same three input modes as run_review so file-list and worktree inputs also reach the
208+
container sandbox instead of silently falling back to the in-process path.
209+
"""
196210
from . import sandbox as sandbox_mod
197211
task_id = task_id or f"cr-{uuid.uuid4().hex[:12]}"
198212
started = time.monotonic()
199-
summary, scan_dir = _materialize(diff_text)
213+
summary, scan_dir, source_type, source_ref = _resolve_input(diff_text, files, repo_path, repo_root)
200214
raw, run = await sandbox_mod.run_container(
201215
scan_dir,
202216
timeout=sandbox_timeout if sandbox_timeout is not None else sandbox_mod.DEFAULT_TIMEOUT_SEC,
@@ -205,7 +219,7 @@ async def run_review_container(
205219
if run.timed_out or run.exit_code not in (0, 1):
206220
exception_dist["sandbox_failure"] = 1
207221
raw = list(raw) + scanners.detect_missing_tests(summary)
208-
return _assemble(task_id, summary, raw, [run], "diff_file", "<diff>", started, exception_dist, None, None)
222+
return _assemble(task_id, summary, raw, [run], source_type, source_ref, started, exception_dist, None, None)
209223

210224

211225
def dedup_thresholds() -> tuple[float, float]:

examples/skills_code_review_agent/run_agent.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010
Dry-run by default: with no API key, FakeReviewModel drives one call to the review_code tool and
1111
summarizes the result — no LLM, no secrets. Set TRPC_AGENT_API_KEY to use a real model instead.
1212
13-
python run_agent.py --fixture 0001_insecure.diff
13+
python run_agent.py --fixture security.diff
14+
python run_agent.py --fixture security.diff --dry-run # force fake model even with a key
1415
"""
1516
from __future__ import annotations
1617

examples/skills_code_review_agent/run_review.py

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -68,16 +68,17 @@ def _parse_args() -> argparse.Namespace:
6868
def _run(args: argparse.Namespace) -> ReviewResult:
6969
runtime = _resolve_runtime(args.runtime)
7070
if args.repo_path:
71-
return run_review(repo_path=args.repo_path, runtime=runtime, sandbox_timeout=args.sandbox_timeout)
72-
if args.files:
73-
return run_review(files=[p.strip() for p in args.files.split(",") if p.strip()],
74-
runtime=runtime,
75-
sandbox_timeout=args.sandbox_timeout)
76-
path = Path(args.diff_file) if args.diff_file else HERE / "fixtures" / "diffs" / args.fixture
77-
diff_text = path.read_text(encoding="utf-8")
71+
src = {"repo_path": args.repo_path}
72+
elif args.files:
73+
src = {"files": [p.strip() for p in args.files.split(",") if p.strip()]}
74+
else:
75+
path = Path(args.diff_file) if args.diff_file else HERE / "fixtures" / "diffs" / args.fixture
76+
src = {"diff_text": path.read_text(encoding="utf-8")}
77+
# Every input mode reaches the resolved runtime — container goes to the async container path so
78+
# --files / --repo-path are not silently downgraded to in-process.
7879
if runtime == "container":
79-
return asyncio.run(run_review_container(diff_text=diff_text, sandbox_timeout=args.sandbox_timeout))
80-
return run_review(diff_text=diff_text, runtime=runtime, sandbox_timeout=args.sandbox_timeout)
80+
return asyncio.run(run_review_container(sandbox_timeout=args.sandbox_timeout, **src))
81+
return run_review(runtime=runtime, sandbox_timeout=args.sandbox_timeout, **src)
8182

8283

8384
async def _persist(result: ReviewResult, db_url: str) -> None:

tests/examples/test_skills_code_review_agent.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,3 +439,21 @@ async def test_status_reflects_blocked(tmp_path) -> None:
439439
assert got["task"].status == "blocked" # not hardcoded "completed"
440440
finally:
441441
await store.close()
442+
443+
444+
def test_run_review_rejects_container_runtime() -> None:
445+
# Sync run_review must reject container (async) loudly, not silently fall back to in-process.
446+
with pytest.raises(ValueError, match="container"):
447+
run_review(diff_text=(_FIXTURES / "security.diff").read_text(), runtime="container")
448+
449+
450+
def test_resolve_input_covers_all_modes(tmp_path) -> None:
451+
# The shared resolver (used by run_review AND run_review_container) handles every input mode,
452+
# so --files / --repo-path reach the container sandbox instead of downgrading to in-process.
453+
from pipeline.engine import _resolve_input
454+
455+
(tmp_path / "m.py").write_text("import os\n")
456+
_, _, st_diff, _ = _resolve_input((_FIXTURES / "security.diff").read_text(), None, None, ".")
457+
_, _, st_files, ref = _resolve_input(None, ["m.py"], None, str(tmp_path))
458+
assert st_diff == "diff_file"
459+
assert st_files == "file_list" and "m.py" in ref

0 commit comments

Comments
 (0)