tes:Feat cr agent#2222
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesAutomated code review agent
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| var buf bytes.Buffer | ||
| for _, args := range [][]string{ // 遍历diff命令 | ||
| {"diff", "HEAD"}, | ||
| {"diff", "--cached"}, |
There was a problem hiding this comment.
git diff HEAD already includes staged changes, so the extra git diff --cached duplicates staged hunks and inflates raw diff input. Use git diff plus git diff --cached, or keep only git diff HEAD.
中文
`git diff HEAD` 已包含暂存改动，因此额外的 `git diff --cached` 会重复暂存 hunk 并放大原始 diff 输入。请改用 `git diff` 加 `git diff --cached`，或只保留 `git diff HEAD`。
| out := &Result{Exceptions: map[string]int{}} | ||
|
|
||
| env, cleanup, err := prepareRunEnv(ctx, opts) | ||
| if err != nil { |
There was a problem hiding this comment.
prepareRunEnv errors are swallowed here, so an unsupported --runtime is reported as a completed review. Return unsupported runtime errors or validate the runtime before execution.
中文
这里会吞掉 `prepareRunEnv` 错误，因此非法 `--runtime` 会被报告为审查完成。请返回非法 runtime 错误，或在执行前校验 runtime。![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/skills_code_review_agent/fixtures/05_db_connection.diff (1)
1-16: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the hunk counts in this fixture.
The
@@ -18,6 +12 @@header does not match the body: it declares 6 old-side and 12 new-side lines, but the hunk contains 5 context lines and 6 added lines. Update the header to match the actual body so the fixture stays parseable and exercises the intended DB-lifecycle case.中文
@@ -18,6 +12 @@头部与正文不一致：它声明旧侧 6 行、新侧 12 行，但实际只有 5 行上下文和 6 行新增。请把头部改成与正文一致，避免 fixture 无法解析，并保持其对数据库生命周期问题的覆盖。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/skills_code_review_agent/fixtures/05_db_connection.diff` around lines 1 - 16, Update the unified-diff hunk header for OrderStore.CreateOrder so its old and new line counts match the actual hunk body: 5 context lines on the old side and 11 lines on the new side, including 6 additions. Preserve the hunk contents and intended database-lifecycle scenario.
🧹 Nitpick comments (8)
examples/skills_code_review_agent/internal/storage/sqlite_test.go (1)
23-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest round-trip coverage gap: permission decisions, sandbox runs, and error paths untested.
This test only exercises
Findings/Warnings/Artifactsround-trip. It doesn't populate or assertPermissionDecisions/SandboxRuns(both persisted and read back insqlite.go), doesn't cover a low-confidence finding that must be reclassified as a warning on read (the exact path flagged insqlite.golines 251-256), and doesn't testGetReviewfor a non-existenttaskID.中文
**测试往返覆盖存在缺口：权限决策、沙箱运行记录、错误路径均未覆盖。**该测试仅验证了
Findings/Warnings/Artifacts的往返。未填充或断言PermissionDecisions/SandboxRuns（sqlite.go中均有持久化和读取逻辑），也未覆盖低置信度 finding 在读取时被重新分类为 warning 的路径（正是sqlite.go第251-256行标记的问题），也没有测试GetReview对不存在taskID的行为。Based on path instructions for
**/*_test.go: "Whether the main path, boundary conditions, error paths, and regression scenarios are covered."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/skills_code_review_agent/internal/storage/sqlite_test.go` around lines 23 - 87, Expand TestSQLiteStoreSaveAndGet to populate and assert PermissionDecisions and SandboxRuns after SaveReview/GetReview, add a low-confidence finding that verifies read-time reclassification into Warnings, and add a GetReview call with an unknown taskID asserting the expected not-found error behavior.Source: Path instructions
examples/skills_code_review_agent/output/02_security/review_report.md (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim leading/trailing whitespace in evidence strings for Markdown generation.
The generated
Evidencestring includes leading whitespace (tabs/spaces), which triggers markdownlint (MD038: spaces inside code span elements) and affects the visual presentation of the generated report. Consider updating the markdown generator (e.g.,internal/report/report.go) to trim whitespace from theEvidencefield before wrapping it in backticks.中文
在生成 Markdown 时裁剪 Evidence 字符串前后的空白字符
生成的 Evidence 字符串包含了前导空白字符（Tab/空格），这会触发 markdownlint 警告（MD038：代码片段内有空格），同时也会影响报告的排版美观。建议更新 Markdown 生成器（如
internal/report/report.go），在用反引号包裹前去掉Evidence字段的空白字符。
examples/skills_code_review_agent/output/02_security/review_report.md#L37-L37: Update the report generator to trim evidence before printing.examples/skills_code_review_agent/output/03_goroutine_leak/review_report.md#L27-L27: Update the report generator to trim evidence before printing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/skills_code_review_agent/output/02_security/review_report.md` at line 37, Update the Markdown generation logic in internal/report/report.go to trim leading and trailing whitespace from each Evidence value before wrapping it in backticks. This root-cause fix should correct the evidence output in examples/skills_code_review_agent/output/02_security/review_report.md lines 37-37 and examples/skills_code_review_agent/output/03_goroutine_leak/review_report.md lines 27-27; do not modify those generated reports directly unless regeneration is required.Source: Linters/SAST tools
examples/skills_code_review_agent/README.md (1)
39-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language hint to the architecture fence block (MD040).
markdownlint flags this fence as missing a language identifier.
🐛 Proposed fix
-``` +```text Input (diff / repo / fixture) → diff parser → rules engine → dedup / partition → PermissionPolicy gate → sandbox (skill_run / go vet / go test) → redact → report + SQLite</details> <details><summary>中文</summary> markdownlint 提示该代码块缺少语言标识（MD040），建议补充如 `text`。 </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@examples/skills_code_review_agent/README.mdaround lines 39 - 44, Update the
architecture diagram’s fenced code block in the README to include the text
language identifier, changing the opening fence to a text-marked fence while
preserving its diagram content.</details> <!-- cr-comment:v1:2069890fb28ab285ae73adfb --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>examples/skills_code_review_agent/DESIGN.md (1)</summary><blockquote> `9-17`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Duplicate list numbering ("4." used twice).** The architecture list numbers 治理层 and Skill 层 both as "4.", so numbering doesn't match the actual 8-layer list described below (`降噪层`/`安全层`/`持久层`/`输出层` are 5-8). <details> <summary>🐛 Proposed fix</summary> ```diff -4. **治理层** (`internal/sandbox`)：PermissionPolicy 拦截高风险命令；仅 allow 的命令进入沙箱。 -4. **Skill 层** (`skills/code-review`)：SKILL.md 描述工作流；`scripts/run_checks.sh` 校验 diff；可选 `go vet`。 -5. **降噪层** (`internal/findings`)：按 file+line+category 去重；confidence < 0.6 进入 warnings。 -6. **安全层** (`internal/redact`)：报告与数据库写入前脱敏 API Key / token / password。 -7. **持久层** (`internal/storage`)：SQLite 六张表，Store 接口可替换 PostgreSQL 等后端。 -8. **输出层** (`internal/report`)：JSON + Markdown，含 findings、治理拦截、沙箱摘要、监控指标。 +4. **治理层** (`internal/sandbox`)：PermissionPolicy 拦截高风险命令；仅 allow 的命令进入沙箱。 +5. **Skill 层** (`skills/code-review`)：SKILL.md 描述工作流；`scripts/run_checks.sh` 校验 diff；可选 `go vet`。 +6. **降噪层** (`internal/findings`)：按 file+line+category 去重；confidence < 0.6 进入 warnings。 +7. **安全层** (`internal/redact`)：报告与数据库写入前脱敏 API Key / token / password。 +8. **持久层** (`internal/storage`)：SQLite 六张表，Store 接口可替换 PostgreSQL 等后端。 +9. **输出层** (`internal/report`)：JSON + Markdown，含 findings、治理拦截、沙箱摘要、监控指标。中文
架构列表中“治理层”和“Skill 层”都编号为“4.”，导致后续编号与实际的 8 层结构不一致，建议顺序重新编号。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/skills_code_review_agent/DESIGN.md` around lines 9 - 17, Correct the architecture list numbering in DESIGN.md so the layers are sequentially numbered 1 through 8: keep 治理层 as 4, number Skill 层 as 5, and increment 降噪层, 安全层, 持久层, and 输出层 to 6–9 only if the list is intended to contain nine entries; otherwise reconcile the stated “8-layer” description by correcting the duplicated or extra layer numbering consistently.Source: Path instructions
examples/skills_code_review_agent/internal/llmreview/parse_test.go (1)
16-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing error-path test for malformed JSON.
All three tests exercise success paths; none verifies that
ParseFindingsreturns an error when the extracted array contains invalid JSON. As per path instructions, "Whether the main path, boundary conditions, error paths, and regression scenarios are covered" for**/*_test.go.func TestParseFindingsInvalidJSON(t *testing.T) { if _, err := ParseFindings(`[{"severity":`); err == nil { t.Fatal("expected error for malformed JSON") } }中文
三个测试均为成功路径，缺少对格式错误 JSON 的错误路径验证。根据路径规范，`**/*_test.go` 应覆盖主路径、边界条件和错误路径。建议补充一个负向测试用例。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/skills_code_review_agent/internal/llmreview/parse_test.go` around lines 16 - 49, Add a malformed-JSON error-path test alongside the existing ParseFindings tests, using TestParseFindingsInvalidJSON to call ParseFindings with an incomplete JSON array and assert that it returns a non-nil error.Source: Path instructions
examples/skills_code_review_agent/internal/sandbox/executor.go (1)
34-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated runtime dispatch between
NewCodeExecutorandnewWorkspaceExecutor.Both functions switch on
opts.Runtimeto construct the same underlying container/E2B/local executors. A future runtime addition updated in only one of the two switches would silently diverge (one path allows it, the other returns "unsupported runtime").中文
`NewCodeExecutor` 与 `newWorkspaceExecutor` 都对 `opts.Runtime` 做几乎相同的 switch 来构造 container/E2B/local 执行器。若未来新增 Runtime 只更新了其中一处，会导致两条路径出现不一致（一处支持、另一处报 "unsupported runtime"）。建议抽取共用的 runtime→构造函数映射。Also applies to: 77-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/skills_code_review_agent/internal/sandbox/executor.go` around lines 34 - 48, Remove the duplicated runtime dispatch by extracting the shared Runtime-to-executor construction logic from NewCodeExecutor and newWorkspaceExecutor into one helper. Update both call paths to reuse that helper, preserving the existing container, E2B, local, timeout, workspace, and unsupported-runtime behavior so future runtime additions require only one dispatch update.examples/skills_code_review_agent/internal/llmreview/parse.go (1)
21-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBracket-matching via
LastIndexis fragile to trailing text.If the model appends any explanation after the array that itself contains a
], extraction breaks (valid JSON becomes truncated/garbled) and a recoverable response turns into a decode error. A streaming decoder naturally stops after the first valid JSON value, regardless of trailing content.♻️ Proposed fix
- start := strings.Index(content, "[") - end := strings.LastIndex(content, "]") - if start < 0 || end <= start { - return nil, nil - } - - var items []findings.Finding - if err := json.Unmarshal([]byte(content[start:end+1]), &items); err != nil { - return nil, fmt.Errorf("decode llm findings: %w", err) - } + start := strings.Index(content, "[") + if start < 0 { + return nil, nil + } + + var items []findings.Finding + if err := json.NewDecoder(strings.NewReader(content[start:])).Decode(&items); err != nil { + return nil, fmt.Errorf("decode llm findings: %w", err) + }中文
若模型在数组后附加包含 `]` 的说明文字，`LastIndex` 会截取到错误位置导致本可恢复的响应变成解码错误。建议使用流式 `json.Decoder`，其在读到第一个合法 JSON 值后即停止，不受尾部文本影响。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/skills_code_review_agent/internal/llmreview/parse.go` around lines 21 - 44, Update ParseFindings to replace the strings.Index/LastIndex bracket extraction with a streaming json.Decoder that starts at the first JSON array and decodes the first valid value, ignoring trailing model text. Preserve the existing empty-input handling, decode error wrapping, and defaulting of empty Finding.Source to "llm".examples/skills_code_review_agent/internal/sandbox/checks.go (1)
19-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo independent implementations of the same check logic risk drift.
runChecksduplicatesscripts/run_checks.sh's validation logic in Go, used as the direct-mode fallback. Since the two are maintained separately, a future change to one (e.g. new ignored-error pattern, line-limit change) without the matching change to the other would make the sandbox produce different pass/fail results depending on which runtime path executed it.中文
`runChecks` 用 Go 重新实现了 `scripts/run_checks.sh` 的校验逻辑，作为直连模式的回退路径。两者分别维护，若日后只更新其中一个（如忽略错误模式、行数上限），会导致沙箱在不同运行时路径下产生不一致的通过/失败结果。建议增加一个共享 fixture 驱动的对齐测试，或在两处添加“需同步维护”的说明。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/skills_code_review_agent/internal/sandbox/checks.go` around lines 19 - 53, Add a shared fixture-driven parity test covering the validation cases handled by runChecks and scripts/run_checks.sh, including ignored-error patterns, line limits, empty input, and Git headers; ensure both implementations produce equivalent pass/fail outcomes. If shared testing is impractical, add explicit synchronization comments at both implementations, but prefer the parity test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/skills_code_review_agent/internal/diff/diff.go`:
- Around line 259-290: Update InferGoPackage to clean the normalized file path
and reject paths that escape the repository, including paths beginning with or
resolving through "..", before invoking lookupGoPackage. Return the existing
empty-package result for rejected paths, while preserving normal .go file
handling and package lookup.
In `@examples/skills_code_review_agent/internal/findings/dedup.go`:
- Around line 24-29: Update the deduplication key in the findings loop to
include each finding’s RuleID along with File, Line, and Category. Preserve the
existing first-occurrence behavior while ensuring distinct rules on the same
file, line, and category are retained.
In `@examples/skills_code_review_agent/internal/llmreview/fake_model.go`:
- Around line 19-39: Protect the step counter in fakeReviewModel from concurrent
access by replacing the plain int field with sync/atomic.Int32 and updating
GenerateContent to atomically increment and read it when selecting the response
content. Add the required sync/atomic import while preserving the existing
first-call and subsequent-call behavior.
In `@examples/skills_code_review_agent/internal/llmreview/review.go`:
- Around line 71-110: Explicitly close the sandbox executor created by
sandbox.NewCodeExecutor before the review flow exits, while preserving the
existing runner cleanup via r.Close(). Use the concrete executor’s Close
lifecycle or an appropriate closeable wrapper, and ensure cleanup occurs on both
normal and error paths after creation.
In `@examples/skills_code_review_agent/internal/rules/rules.go`:
- Around line 118-159: Update the finding deduplication logic to include each
finding’s RuleID in its deduplication key, preserving distinct CONC-001 and
CONC-002 findings for the same file, line, and category. Locate the code that
currently keys deduplication by file:line:category and extend only that key;
keep the existing rule matching behavior in matchGoroutineLeak and
matchContextPropagation unchanged.
In `@examples/skills_code_review_agent/internal/sandbox/runner_test.go`:
- Around line 51-69: Replace the Go 1.24-only testing.T.Context() call in
TestRunSandboxFailureDoesNotPanic with a Go 1.23-compatible context source such
as context.Background(), adding the required context import and leaving the test
assertions unchanged.
In `@examples/skills_code_review_agent/internal/sandbox/runner.go`:
- Around line 33-35: Update denyRE so its curl and wget alternatives match any
number of command-line flags or arguments before the URL and the pipe to
sh/bash, including forms such as curl -sSL URL|sh. Preserve the existing
case-insensitive blocking behavior and ensure compound commands checked by
checkPermission cannot bypass the deny rule through these prefixed options.
- Around line 107-115: Update Run and runGoCommand so prepareRunEnv failures for
any isolated runtime, including RuntimeContainer and RuntimeE2B, are blocked
from falling through to direct host execution; return the existing failure
behavior instead. Apply the equivalent isIsolatedRuntime guard to the
runGoCommand path and ensure executePlanned preserves isolation when environment
setup fails. Count workspace_error for RuntimeE2B as well as RuntimeContainer in
Run.
- Around line 197-231: Update executePlanned so the elapsed DurationMs is
applied to the RunRecord that each execution path returns, rather than only to
the local rec modified by defer. Preserve the existing duration calculation and
ensure all branches, including workspace, direct, Go command, and
unsupported-command returns, expose the measured per-run duration to
SandboxRunSummary and persisted sandbox run records.
---
Outside diff comments:
In `@examples/skills_code_review_agent/fixtures/05_db_connection.diff`:
- Around line 1-16: Update the unified-diff hunk header for
OrderStore.CreateOrder so its old and new line counts match the actual hunk
body: 5 context lines on the old side and 11 lines on the new side, including 6
additions. Preserve the hunk contents and intended database-lifecycle scenario.
---
Nitpick comments:
In `@examples/skills_code_review_agent/DESIGN.md`:
- Around line 9-17: Correct the architecture list numbering in DESIGN.md so the
layers are sequentially numbered 1 through 8: keep 治理层 as 4, number Skill 层 as
5, and increment 降噪层, 安全层, 持久层, and 输出层 to 6–9 only if the list is intended to
contain nine entries; otherwise reconcile the stated “8-layer” description by
correcting the duplicated or extra layer numbering consistently.
In `@examples/skills_code_review_agent/internal/llmreview/parse_test.go`:
- Around line 16-49: Add a malformed-JSON error-path test alongside the existing
ParseFindings tests, using TestParseFindingsInvalidJSON to call ParseFindings
with an incomplete JSON array and assert that it returns a non-nil error.
In `@examples/skills_code_review_agent/internal/llmreview/parse.go`:
- Around line 21-44: Update ParseFindings to replace the strings.Index/LastIndex
bracket extraction with a streaming json.Decoder that starts at the first JSON
array and decodes the first valid value, ignoring trailing model text. Preserve
the existing empty-input handling, decode error wrapping, and defaulting of
empty Finding.Source to "llm".
In `@examples/skills_code_review_agent/internal/sandbox/checks.go`:
- Around line 19-53: Add a shared fixture-driven parity test covering the
validation cases handled by runChecks and scripts/run_checks.sh, including
ignored-error patterns, line limits, empty input, and Git headers; ensure both
implementations produce equivalent pass/fail outcomes. If shared testing is
impractical, add explicit synchronization comments at both implementations, but
prefer the parity test.
In `@examples/skills_code_review_agent/internal/sandbox/executor.go`:
- Around line 34-48: Remove the duplicated runtime dispatch by extracting the
shared Runtime-to-executor construction logic from NewCodeExecutor and
newWorkspaceExecutor into one helper. Update both call paths to reuse that
helper, preserving the existing container, E2B, local, timeout, workspace, and
unsupported-runtime behavior so future runtime additions require only one
dispatch update.
In `@examples/skills_code_review_agent/internal/storage/sqlite_test.go`:
- Around line 23-87: Expand TestSQLiteStoreSaveAndGet to populate and assert
PermissionDecisions and SandboxRuns after SaveReview/GetReview, add a
low-confidence finding that verifies read-time reclassification into Warnings,
and add a GetReview call with an unknown taskID asserting the expected not-found
error behavior.
In `@examples/skills_code_review_agent/output/02_security/review_report.md`:
- Line 37: Update the Markdown generation logic in internal/report/report.go to
trim leading and trailing whitespace from each Evidence value before wrapping it
in backticks. This root-cause fix should correct the evidence output in
examples/skills_code_review_agent/output/02_security/review_report.md lines
37-37 and
examples/skills_code_review_agent/output/03_goroutine_leak/review_report.md
lines 27-27; do not modify those generated reports directly unless regeneration
is required.
In `@examples/skills_code_review_agent/README.md`:
- Around line 39-44: Update the architecture diagram’s fenced code block in the
README to include the text language identifier, changing the opening fence to a
text-marked fence while preserving its diagram content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99914433-f7a4-4b40-b920-0faa8b12557a
⛔ Files ignored due to path filters (1)
examples/skills_code_review_agent/go.sumis excluded by!**/*.sum
📒 Files selected for processing (59)
examples/skills_code_review_agent/DESIGN.mdexamples/skills_code_review_agent/README.mdexamples/skills_code_review_agent/fixtures/01_clean.diffexamples/skills_code_review_agent/fixtures/02_security.diffexamples/skills_code_review_agent/fixtures/03_goroutine_leak.diffexamples/skills_code_review_agent/fixtures/04_resource_leak.diffexamples/skills_code_review_agent/fixtures/05_db_connection.diffexamples/skills_code_review_agent/fixtures/06_missing_test.diffexamples/skills_code_review_agent/fixtures/07_duplicate_finding.diffexamples/skills_code_review_agent/fixtures/08_sandbox_fail.diffexamples/skills_code_review_agent/go.modexamples/skills_code_review_agent/internal/diff/diff.goexamples/skills_code_review_agent/internal/diff/diff_test.goexamples/skills_code_review_agent/internal/findings/dedup.goexamples/skills_code_review_agent/internal/findings/finding.goexamples/skills_code_review_agent/internal/findings/findings_test.goexamples/skills_code_review_agent/internal/llmreview/fake_model.goexamples/skills_code_review_agent/internal/llmreview/parse.goexamples/skills_code_review_agent/internal/llmreview/parse_test.goexamples/skills_code_review_agent/internal/llmreview/review.goexamples/skills_code_review_agent/internal/pipeline/pipeline.goexamples/skills_code_review_agent/internal/redact/findings.goexamples/skills_code_review_agent/internal/redact/redact.goexamples/skills_code_review_agent/internal/redact/redact_test.goexamples/skills_code_review_agent/internal/report/report.goexamples/skills_code_review_agent/internal/rules/rules.goexamples/skills_code_review_agent/internal/rules/rules_test.goexamples/skills_code_review_agent/internal/sandbox/checks.goexamples/skills_code_review_agent/internal/sandbox/container.goexamples/skills_code_review_agent/internal/sandbox/executor.goexamples/skills_code_review_agent/internal/sandbox/runner.goexamples/skills_code_review_agent/internal/sandbox/runner_test.goexamples/skills_code_review_agent/internal/storage/schema.sqlexamples/skills_code_review_agent/internal/storage/sqlite.goexamples/skills_code_review_agent/internal/storage/sqlite_test.goexamples/skills_code_review_agent/internal/storage/store.goexamples/skills_code_review_agent/main.goexamples/skills_code_review_agent/output/01_clean/review_report.jsonexamples/skills_code_review_agent/output/01_clean/review_report.mdexamples/skills_code_review_agent/output/02_security/review_report.jsonexamples/skills_code_review_agent/output/02_security/review_report.mdexamples/skills_code_review_agent/output/03_goroutine_leak/review_report.jsonexamples/skills_code_review_agent/output/03_goroutine_leak/review_report.mdexamples/skills_code_review_agent/output/04_resource_leak/review_report.jsonexamples/skills_code_review_agent/output/04_resource_leak/review_report.mdexamples/skills_code_review_agent/output/05_db_connection/review_report.jsonexamples/skills_code_review_agent/output/05_db_connection/review_report.mdexamples/skills_code_review_agent/output/06_missing_test/review_report.jsonexamples/skills_code_review_agent/output/06_missing_test/review_report.mdexamples/skills_code_review_agent/output/07_duplicate_finding/review_report.jsonexamples/skills_code_review_agent/output/07_duplicate_finding/review_report.mdexamples/skills_code_review_agent/output/08_sandbox_fail/review_report.jsonexamples/skills_code_review_agent/output/08_sandbox_fail/review_report.mdexamples/skills_code_review_agent/output/example_review_report.jsonexamples/skills_code_review_agent/output/example_review_report.mdexamples/skills_code_review_agent/pipeline_test.goexamples/skills_code_review_agent/skills/code-review/SKILL.mdexamples/skills_code_review_agent/skills/code-review/docs/rules.mdexamples/skills_code_review_agent/skills/code-review/scripts/run_checks.sh
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2222 +/- ##
===================================================
+ Coverage 89.88304% 89.88356% +0.00052%
===================================================
Files 1138 1144 +6
Lines 196729 198637 +1908
===================================================
+ Hits 176826 178542 +1716
- Misses 12465 12594 +129
- Partials 7438 7501 +63
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| agentName, | ||
| llmagent.WithModel(mdl), | ||
| llmagent.WithSkills(repo), | ||
| llmagent.WithSkillToolProfile(llmagent.SkillToolProfileFull), |
There was a problem hiding this comment.
SkillToolProfileFull exposes skill_run, but that execution path is not checked by this example's PermissionPolicy, so LLM mode can invoke arbitrary skill commands outside the advertised allow/deny and env whitelist. Use SkillToolProfileKnowledgeOnly here, or expose a purpose-built runner that applies the same permission policy before any command execution.
中文
`SkillToolProfileFull` 会暴露 `skill_run`，但这条执行路径没有走这个示例里的 `PermissionPolicy`，所以 LLM 模式可以绕过宣称的允许/拒绝策略和环境变量白名单去调用任意 skill 命令。这里建议改用 `SkillToolProfileKnowledgeOnly`，或者只暴露一个会先执行同一套权限校验的专用 runner。
| if err != nil { | ||
| return fmt.Errorf("resolve repo path: %w", err) | ||
| } | ||
| if err := exec.PutDirectory(ctx, ws, repo, filepath.Join("work", "repo")); err != nil { |
There was a problem hiding this comment.
PutDirectory stages the entire host repository for --repo-path, which can copy .git, ignored/untracked files, or local secrets into the sandbox. Stage a sanitized tree instead, for example tracked files from git archive/git ls-files plus explicit changed files, and exclude .git, ignored files, and secret patterns.
中文
`PutDirectory` 会把 `--repo-path` 指向的整个宿主机仓库放进 sandbox，这可能把 `.git`、被 ignore 的文件、未跟踪产物或本地密钥一起带进去。建议改成只放入清理过的文件树，例如 `git archive`/`git ls-files` 得到的 tracked files 加上明确选择的改动文件，并排除 `.git`、ignore 文件和常见密钥模式。
| case strings.HasPrefix(line, "diff --git "): | ||
| flushFile() | ||
| current = &FileChange{} | ||
| case strings.HasPrefix(line, "--- "): |
There was a problem hiding this comment.
--diff-file can be a standard unified diff, but this parser only creates current after diff --git; a patch starting with --- old / +++ new produces no FileChange. Initialize a file change on --- when current is nil, attach the +++ path, and add coverage for non-git unified diffs.
中文
`--diff-file` 声称可以接收普通 unified diff，但这里只有遇到 `diff --git` 才会创建 `current`；如果 patch 直接以 `--- old` / `+++ new` 开头，就不会产生任何 `FileChange`。建议在 `current` 为空时遇到 `--- ` 也初始化文件变更，再用 `+++ ` 绑定新路径，并补上非 git unified diff 的测试。
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
examples/skills_code_review_agent/internal/sandbox/repo_stage.go (1)
145-151: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAvoid trimming spaces from valid file paths.
git ls-files -zseparates entries precisely with null bytes (\x00).strings.TrimSpaceis unnecessary and will corrupt valid file paths that intentionally begin or end with a space, preventing them from being staged.中文
避免清理有效文件路径的空格。
git ls-files -z严格使用空字节 (\x00) 分隔路径。在此处使用strings.TrimSpace是多余的，且会破坏首尾原本就有空格的合法文件路径，导致它们无法被正确读取和载入沙箱。♻️ Proposed fix
for _, p := range parts { - p = strings.TrimSpace(p) if p == "" { continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/skills_code_review_agent/internal/sandbox/repo_stage.go` around lines 145 - 151, Update the file-list parsing loop to preserve leading and trailing spaces in each path: remove the strings.TrimSpace call while retaining null-entry filtering and filepath.ToSlash conversion in the existing parsing function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/skills_code_review_agent/internal/diff/diff_test.go`:
- Around line 38-39: Strengthen the assertions in the diff parsing test around
d.Files[0].Hunks[0].AddedLines by verifying the sole added line has Line equal
to 2 and content import "fmt". Keep the existing count checks while covering the
AddedLine.Line contract.
In `@examples/skills_code_review_agent/internal/sandbox/repo_stage.go`:
- Around line 82-88: Update the file collection logic around os.Stat to use
os.Lstat, then accept entries only when the call succeeds and
info.Mode().IsRegular() is true. This must exclude symbolic links and
directories before appending rel to out, preventing os.ReadFile from following
links to host files.
- Around line 177-181: Update isExcludedRepoPath to reject any non-local path by
applying filepath.IsLocal after normalization and before allowing the path;
retain the existing empty and .git exclusions so only safe, repository-relative
paths proceed.
---
Nitpick comments:
In `@examples/skills_code_review_agent/internal/sandbox/repo_stage.go`:
- Around line 145-151: Update the file-list parsing loop to preserve leading and
trailing spaces in each path: remove the strings.TrimSpace call while retaining
null-entry filtering and filepath.ToSlash conversion in the existing parsing
function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1fe5252-8601-4b7e-8c85-328a921d9be5
📒 Files selected for processing (6)
examples/skills_code_review_agent/internal/diff/diff.goexamples/skills_code_review_agent/internal/diff/diff_test.goexamples/skills_code_review_agent/internal/llmreview/review.goexamples/skills_code_review_agent/internal/sandbox/repo_stage.goexamples/skills_code_review_agent/internal/sandbox/runner.goexamples/skills_code_review_agent/internal/sandbox/runner_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/skills_code_review_agent/internal/llmreview/review.go
- examples/skills_code_review_agent/internal/sandbox/runner.go
- examples/skills_code_review_agent/internal/diff/diff.go
| func isExcludedRepoPath(rel string) bool { | ||
| rel = filepath.ToSlash(strings.TrimSpace(rel)) | ||
| if rel == "" || rel == ".git" || strings.HasPrefix(rel, ".git/") { | ||
| return true | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prevent path traversal vulnerabilities.
Paths extracted from diffRaw are appended to repoPath. An attacker could submit a patch with a path like ../../../etc/passwd to read arbitrary host files and stage them into the sandbox. Use filepath.IsLocal (available in Go 1.20+) to ensure the path stays safely within the repository tree.
中文
防止路径穿越漏洞。
从 diffRaw 提取的路径会在宿主机上与 repoPath 拼接。恶意用户可能通过包含 ../../../etc/passwd 的 diff 读取任意宿主机文件并将其载入沙箱。请使用 filepath.IsLocal（Go 1.20+ 原生提供）来严格确保路径安全地限制在仓库目录内。
🛡️ Proposed fix
func isExcludedRepoPath(rel string) bool {
rel = filepath.ToSlash(strings.TrimSpace(rel))
- if rel == "" || rel == ".git" || strings.HasPrefix(rel, ".git/") {
+ if rel == "" || !filepath.IsLocal(rel) || rel == ".git" || strings.HasPrefix(rel, ".git/") {
return true
}
base := filepath.Base(rel)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func isExcludedRepoPath(rel string) bool { | |
| rel = filepath.ToSlash(strings.TrimSpace(rel)) | |
| if rel == "" || rel == ".git" || strings.HasPrefix(rel, ".git/") { | |
| return true | |
| } | |
| func isExcludedRepoPath(rel string) bool { | |
| rel = filepath.ToSlash(strings.TrimSpace(rel)) | |
| if rel == "" || !filepath.IsLocal(rel) || rel == ".git" || strings.HasPrefix(rel, ".git/") { | |
| return true | |
| } |
| func isExcludedRepoPath(rel string) bool { | |
| rel = filepath.ToSlash(strings.TrimSpace(rel)) | |
| if rel == "" || rel == ".git" || strings.HasPrefix(rel, ".git/") { | |
| return true | |
| } | |
| func isExcludedRepoPath(rel string) bool { | |
| rel = filepath.ToSlash(strings.TrimSpace(rel)) | |
| if rel == "" || !filepath.IsLocal(rel) || rel == ".git" || strings.HasPrefix(rel, ".git/") { | |
| return true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/skills_code_review_agent/internal/sandbox/repo_stage.go` around
lines 177 - 181, Update isExcludedRepoPath to reject any non-local path by
applying filepath.IsLocal after normalization and before allowing the path;
retain the existing empty and .git exclusions so only safe, repository-relative
paths proceed.
| env, cleanup, err := prepareRunEnv(ctx, opts) | ||
| if err != nil { | ||
| if isIsolatedRuntime(opts.Runtime) { | ||
| return nil, fmt.Errorf("prepare workspace: %w", err) |
There was a problem hiding this comment.
--runtime=container/e2b setup failures return an error from Run, so the pipeline exits before persisting the review report or recording a sandbox failure. Return a completed Result with a failed run/exception count for setup errors while still refusing local fallback for isolated runtimes.
中文
`--runtime=container`/`e2b` 的环境准备失败会直接从 `Run` 返回 error，导致整个流水线在写 review report 或记录 sandbox failure 之前退出。建议对 setup error 返回一个完成态 `Result`，里面记录失败的 run/exception 计数，同时仍然禁止 isolated runtime 回退到 local。
| if err := scanner.Err(); err != nil { | ||
| return "", err.Error(), 2 | ||
| } | ||
| if !hasGitHeader { |
There was a problem hiding this comment.
runChecks still requires diff --git, so valid plain unified diffs accepted by the parser are rejected as sandbox failures. Accept ---/+++ headers plus hunks here and in skills/code-review/scripts/run_checks.sh, with a regression for non-git unified diffs.
中文
`runChecks` 仍然强制要求 `diff --git`，所以 parser 已经支持的普通 unified diff 会在 sandbox 校验阶段被误判失败。建议这里和 `skills/code-review/scripts/run_checks.sh` 同时接受 `---`/`+++` 头和 hunk，并补一个非 git unified diff 的回归测试。
| flagDBPath = flag.String("db-path", "reviews.db", "sqlite database path") | ||
| flagOutputDir = flag.String("output-dir", "output", "directory for review reports") | ||
| flagSkillsRoot = flag.String("skills-root", "skills", "skills root directory") | ||
| flagRuntime = flag.String("runtime", "local", "sandbox runtime: local|container|skip") |
There was a problem hiding this comment.
--runtime defaults to local, so --repo-path runs go vet and go test on the host unless the caller opts into a sandbox. Default repo execution to an isolated runtime or require an explicit --runtime=local development override.
中文
`--runtime` 默认是 `local`，所以传 `--repo-path` 时会直接在宿主机上跑 `go vet` 和 `go test`，除非调用方主动切到 sandbox。建议 repo 执行默认使用隔离 runtime，或者要求开发调试场景显式传 `--runtime=local`。4 participants
Fixes #2004
Summary
在
examples/skills_code_review_agent/新增 Go 自动 Code Review Agent 示例，将 tRPC-Agent-Go 的 Skill、Workspace 沙箱、Permission 门控、SQLite 持久化与结构化报告串联为一条可验证的审查链路。--dry-run=false使用 llmagent + code-review Skill；--fake-model无需 API Key 可验证 Agent 编排Architecture
能力对照
Test plan
cd examples/skills_code_review_agent && go test ./...go run . --fixture 01_clean --fake-model运行示例