examples/code_review_agent: add sandboxed Go code review agent#2263
examples/code_review_agent: add sandboxed Go code review agent#2263zezhen0222 wants to merge 2 commits into
Conversation
Add an auditable code review pipeline with governed sandbox execution, normalized SQLite persistence, redacted reports, and deterministic fixtures. Fixes trpc-group#2004
📝 WalkthroughEnglishChange overview
Compatibility and behavioral risks
Recommended validation
中文变更概览
兼容性与行为风险
建议验证步骤
WalkthroughAdds a new EN: A new self-contained example implements diff parsing, rule detection, permission-governed sandboxed checks, SQLite storage, and report generation for automated Go code review, with extensive unit tests and fixtures. ZH (collapsed): 新增独立示例，实现了差异解析、规则检测、权限治理的沙箱检查、SQLite 存储以及自动化 Go 代码评审的报告生成，并附带大量单测和 fixture。 ChangesCode Review Agent Example
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
| type engineFactory func(context.Context, Config, string) (codeexecutor.Engine, func() error, error) | ||
|
|
||
| var containerFactory engineFactory = func(_ context.Context, _ Config, baseDir string) (codeexecutor.Engine, func() error, error) { | ||
| executor, err := containerexec.New(containerexec.WithDockerFilePath(filepath.Join(baseDir, "sandbox"))) |
There was a problem hiding this comment.
WithDockerFilePath builds this Dockerfile into the shared python:3.9-slim tag, which overwrites the default Python executor image. Set a dedicated image tag with WithContainerConfig.
中文
`WithDockerFilePath` 会把这个 Dockerfile 构建到共享的 `python:3.9-slim` 标签，从而覆盖默认 Python executor 镜像。用 `WithContainerConfig` 设置专用镜像标签。
| "CGO_ENABLED": "0", "GOTOOLCHAIN": "local", "GOPROXY": "off", | ||
| } | ||
| } | ||
| return map[string]string{"PATH": "/usr/local/go/bin:/usr/local/bin:/usr/bin:/bin", "HOME": "/tmp", "TMPDIR": "/tmp", "GOCACHE": "/tmp/go-cache", "GOMODCACHE": "/tmp/go-mod", "CGO_ENABLED": "0", "GOTOOLCHAIN": "local", "GONOSUMDB": "*", "GOPROXY": "off"} |
There was a problem hiding this comment.
The default container cannot run Go checks for modules with external dependencies because GOPROXY=off points at an empty GOMODCACHE. Stage a dependency cache or skip Go checks when dependencies are absent.
中文
默认容器无法为带外部依赖的模块运行 Go 检查，因为 `GOPROXY=off` 指向空的 `GOMODCACHE`。放入依赖缓存，或在缺少依赖时跳过 Go 检查。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2263 +/- ##
===================================================
- Coverage 89.86515% 89.84955% -0.01561%
===================================================
Files 1148 1149 +1
Lines 202302 203321 +1019
===================================================
+ Hits 181799 182683 +884
- Misses 12815 12921 +106
- Partials 7688 7717 +29
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
examples/code_review_agent/internal/review/rules_test.go-50-53 (1)
50-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not discard fixture-loading errors.
If
clean.diffis missing or unreadable, ignored errors produce an empty input;ParseUnifiedDiff("")succeeds and the clean-fixture test can falsely pass. Check bothexampleDirandos.ReadFileerrors in these tests.中文
不要忽略 fixture 加载错误。
如果
clean.diff缺失或无法读取，被忽略的错误会产生空输入；ParseUnifiedDiff("")会成功，因此 clean fixture 测试可能错误通过。请在这些测试中检查exampleDir和os.ReadFile的错误。As per path instructions, tests must be stable and assertions must actually verify the intended behavior.
Also applies to: 63-66
🤖 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/code_review_agent/internal/review/rules_test.go` around lines 50 - 53, Update TestCleanFixtureHasNoObservation and the additionally affected fixture-loading tests to check and fail on errors returned by exampleDir and os.ReadFile before parsing the fixture. Preserve the existing ParseUnifiedDiff assertions, ensuring missing or unreadable fixtures cannot be treated as empty input.Source: Path instructions
examples/code_review_agent/internal/review/rules.go-223-233 (1)
223-233: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAssociate changed tests with the production changes they cover.
A single changed
_test.goanywhere causesmissingTeststo return false for every production Go file. Unrelated or deleted tests therefore suppress the human-review observation. Match tests at least by package/directory, and retain file change status so deleted tests do not count as coverage.中文
请将变更测试与其覆盖的生产代码关联起来。
当前只要任意位置存在一个变更的
_test.go，missingTests就会对所有生产 Go 文件返回 false。因此，无关测试或被删除的测试都会错误地抑制人工审查提示。至少应按包或目录关联测试，并保留文件变更状态，避免将删除的测试计为覆盖。As per path instructions, test-gap analysis should validate externally relevant behavior without allowing unrelated changes to suppress findings.
🤖 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/code_review_agent/internal/review/rules.go` around lines 223 - 233, Update missingTests to associate changed tests with production Go files by package or directory rather than using any repository-wide _test.go file. Preserve each file’s change status so deleted tests are excluded from coverage, and ensure unrelated test changes cannot suppress findings for production files without matching active tests.Source: Path instructions
examples/code_review_agent/internal/review/input.go-73-82 (1)
73-82: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftEnforce size limits while reading, not after buffering.
os.ReadFileandcmd.Outputcan consume the complete file or Git output before the limit is checked; the file-list itself is also unbounded. A large or changing input can therefore exhaust memory despitemaxInputBytes. Use limited readers or bounded command pipes and reject atmaxInputBytes+1.中文
应在读取过程中执行大小限制，而不是在完整缓冲后检查。
os.ReadFile和cmd.Output会先读取完整文件或 Git 输出，文件列表本身也没有限制。因此，大型或并发变化的输入仍可能在触发maxInputBytes检查前耗尽内存。请使用限流 reader 或受限命令管道，并在读取到maxInputBytes+1时拒绝输入。As per path instructions, external input and resource boundaries must remain bounded. This also conflicts with the PR objective requiring bounded inputs and output limits.
Also applies to: 90-101, 124-129
🤖 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/code_review_agent/internal/review/input.go` around lines 73 - 82, Update readBounded and the related command-output and file-list reading paths to enforce maxInputBytes during streaming rather than after full buffering. Replace os.ReadFile and cmd.Output usage with limited readers or bounded pipes that detect and reject maxInputBytes+1 bytes, including the file-list itself, while preserving existing error handling and returned content for inputs within the limit.Source: Path instructions
examples/code_review_agent/internal/review/rules.go-32-32 (1)
32-32: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestrict command-injection findings to actual shell interpretation.
The regex flags any
exec.Commandcall containing+orfmt.Sprintf, including safe direct execution such asexec.Command("git", "show", "HEAD:"+path). No shell parses that argument, so reporting high-confidence command injection is incorrect. Match dynamic content specifically in ash -c/bash -ccommand string and add a direct-exec negative test.中文
请仅在确实存在 shell 解释时报告命令注入。
该正则会标记任何包含
+或fmt.Sprintf的exec.Command，包括exec.Command("git", "show", "HEAD:"+path)这类安全的直接执行。此时没有 shell 解析参数，因此将其判定为高置信度命令注入并不准确。请仅匹配传入sh -c/bash -c的动态命令字符串，并增加直接执行的反向测试。As per path instructions, security findings must distinguish the precise threat model for untrusted command arguments.
Also applies to: 61-64
🤖 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/code_review_agent/internal/review/rules.go` at line 32, Restrict the dynamicShell rule in the rules configuration to report only dynamically constructed command strings passed to sh -c or bash -c, removing the broad + and fmt.Sprintf alternatives that match safe direct execution. Add a negative test covering direct exec.Command arguments such as git show with a dynamic path, and retain positive coverage for dynamically built shell command strings.Source: Path instructions
examples/code_review_agent/internal/review/input.go-161-167 (1)
161-167: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the synthesized hunk count match the emitted lines.
For newline-terminated content,
strings.Count(contents, "\n")+1reports one more line than the loop emits afterTrimSuffix. An empty file is also emitted as one added blank line. This produces malformed synthetic diffs and incorrect summaries.中文
请确保合成 hunk 的行数与实际输出一致。
对于以换行符结尾的内容，
strings.Count(contents, "\n")+1比TrimSuffix后循环实际输出的行数多一行；空文件还会被表示为新增一个空白行。这会生成格式错误的合成 diff，并导致汇总不准确。As per path instructions, file-list inputs must preserve correct diff semantics.
🤖 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/code_review_agent/internal/review/input.go` around lines 161 - 167, Update the synthetic diff generation around the hunk header and line-emission loop so the declared added-line count exactly matches the lines written. Handle newline-terminated contents without counting the trimmed trailing line, and represent empty files with zero added lines rather than an added blank line; preserve correct file-list diff semantics.Source: Path instructions
examples/code_review_agent/internal/review/rules.go-208-220 (1)
208-220: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not deduplicate distinct rule IDs.
findingLocationKeyomitsRuleID, so two independent findings in the same category on one line collapse to whichever has higher confidence. Their fingerprints already distinguish the rules, yet the audit record incorrectly labels the discarded issue as equivalent. IncludeRuleIDin the dedupe key and reserve duplicate removal for repeated instances of the same rule.中文
不要将不同规则 ID 的发现去重。
findingLocationKey未包含RuleID，因此同一行、同一类别中的两个独立问题会只保留置信度较高者。指纹已经区分这些规则，但审计记录却会错误地将被丢弃的问题标记为等价项。请将RuleID纳入去重键，仅移除同一规则的重复实例。As per path instructions, structured findings and governance decisions must preserve auditable behavioral distinctions. This also conflicts with the PR objective requiring structured rule IDs and auditable deduplication.
🤖 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/code_review_agent/internal/review/rules.go` around lines 208 - 220, Update findingLocationKey to include Finding.RuleID in the deduplication key, while preserving the existing file, line, and category components. Keep selectBestFindings’ confidence-based selection unchanged so only repeated instances of the same rule are deduplicated.Source: Path instructions
examples/code_review_agent/internal/review/input.go-203-218 (1)
203-218: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not parse
diff --gitpaths withstrings.Fields.Valid Git filenames may contain spaces. Splitting the header makes
b/foo bar.gobecomeb/foo, which is immediately added tofiles; the later+++header then adds the real path, yielding a ghost file and incorrect counts. Parse quoted/path-aware headers, or defer registration until authoritative---/+++headers are processed.中文
不要使用
strings.Fields解析diff --git路径。合法的 Git 文件名可能包含空格。当前拆分会将
b/foo bar.go解析成b/foo并立即加入files，后续+++又加入真实路径，从而产生幽灵文件和错误计数。请使用支持引号及路径的解析方式，或等待权威的---/+++头后再登记文件。As per path instructions, parsed locations and public review results must remain semantically accurate.
🤖 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/code_review_agent/internal/review/input.go` around lines 203 - 218, Update the diff-header parsing in the input-processing loop so `diff --git` paths are not split with `strings.Fields`, which truncates filenames containing spaces and registers ghost entries. Use quoted/path-aware parsing, or defer adding entries from `headerFile` until authoritative `---`/`+++` headers; preserve accurate `files` registration and review locations for all valid paths.Source: Path instructions
examples/code_review_agent/go.mod-25-25 (1)
25-25: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBump the vulnerable example dependencies
The module graph inexamples/code_review_agent/go.modstill includesgithub.com/docker/docker v28.4.0+incompatible,go.opentelemetry.io/otel/sdk v1.38.0, andgoogle.golang.org/grpc v1.67.0; move these to patched releases, or prune them if they’re only transitive.中文
examples/code_review_agent/go.mod的依赖图仍包含github.com/docker/docker v28.4.0+incompatible、go.opentelemetry.io/otel/sdk v1.38.0和google.golang.org/grpc v1.67.0；请升级到已修补版本，或在它们只是传递依赖时移除。🤖 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/code_review_agent/go.mod` at line 25, Update the dependency declarations in the example module’s go.mod for github.com/docker/docker, go.opentelemetry.io/otel/sdk, and google.golang.org/grpc to patched releases; if any are only transitive, remove their explicit requirements and regenerate the module graph accordingly.Source: Linters/SAST tools
examples/code_review_agent/internal/review/sandbox.go-158-162 (1)
158-162: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound workspace cleanup after cancellation.
context.WithoutCancel(ctx)removes the deadline, so a stalled remote cleanup can preventrunfrom returning indefinitely. Use a fresh context with a short cleanup timeout and record cleanup failures.中文
为工作区清理设置超时。
context.WithoutCancel(ctx)会移除截止时间；远程清理一旦卡住，run将无限期无法返回。请使用带短超时的新 context，并记录清理失败。As per path instructions, background operations must stop promptly and resource cleanup must remain bounded.
🤖 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/code_review_agent/internal/review/sandbox.go` around lines 158 - 162, Update the workspace cleanup defer around Manager().Cleanup to use a fresh context with a short timeout rather than context.WithoutCancel(ctx), ensuring cleanup cannot delay run indefinitely. Capture and record any Cleanup failure, and release the timeout context after cleanup completes.Source: Path instructions
examples/code_review_agent/scripts/run_fixtures.sh-8-19 (1)
8-19: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winIsolate each fixture run from previous output.
Re-running with the same output directory leaves multiple task reports.
find | headcan then summarize a stale report, and underpipefailmay fail whenfindreceives SIGPIPE. Clean or create a unique per-run directory and consume the exact report path produced by that invocation.中文
将每次 fixture 运行与历史输出隔离。
重复使用相同输出目录会留下多个任务报告。
find | head可能读取旧报告，并可能因find收到 SIGPIPE 而在pipefail下失败。请清理目录或创建独立运行目录，并使用本次命令生成的准确报告路径。As per path instructions, examples and fixture runners must be reproducible and safe to execute repeatedly.
🤖 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/code_review_agent/scripts/run_fixtures.sh` around lines 8 - 19, Update the fixture loop in run_fixtures.sh to isolate each invocation from prior output by cleaning or using a unique per-run directory for each fixture. Replace the find/head report discovery with the exact review_report.json path produced by that fixture run, preserving the existing jq summary behavior and avoiding pipefail issues.Source: Path instructions
examples/code_review_agent/internal/review/sandbox.go-209-217 (1)
209-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve timeout and executor-error precedence for
staticcheck.A timed-out
staticcheckcommonly has exit code-1; Line 215 then overwrites the failure astool_unavailable. Only classify an explicit command-not-found result when the run has not already timed out or failed at the executor layer. Add that combined case toTestExecuteClassifiesResults.中文
保留
staticcheck超时及执行器错误的优先级。超时的
staticcheck通常会返回-1；第 215 行随后会将其覆盖成tool_unavailable。仅在没有超时或执行器错误时，才应识别明确的“命令不存在”结果，并补充组合场景测试。As per path instructions, sandbox errors must remain distinguishable to audit consumers.
🤖 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/code_review_agent/internal/review/sandbox.go` around lines 209 - 217, Update the staticcheck classification in the result-processing flow so “tool_unavailable” is assigned only for an explicit command-not-found result when the run has not timed out or failed at the executor layer; preserve timeout and executor-error statuses and error types. Extend TestExecuteClassifiesResults with combined staticcheck timeout and executor-error cases, ensuring sandbox errors remain distinguishable.Source: Path instructions
examples/code_review_agent/internal/review/report.go-124-137 (1)
124-137: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEscape every untrusted Markdown table field.
escapeCellonly redacts evidence; it does not escape pipes, newlines, backticks, or raw HTML, and the other fields bypass it entirely. A model-generated finding can therefore break rows or spoof additional report content. Escape every table value after redaction.中文
转义 Markdown 表格中的所有不可信字段。
escapeCell仅对证据脱敏，并未转义竖线、换行、反引号或原始 HTML；其他字段甚至未调用它。模型生成的 finding 因此可以破坏表格或伪造额外报告内容。请在脱敏后转义所有表格字段。As per path instructions, model-driven output must be treated as untrusted while preserving report accuracy.
🤖 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/code_review_agent/internal/review/report.go` around lines 124 - 137, Update escapeCell and renderFindingTable so every untrusted table field—Severity, Category, File, Line-derived location, Title, Evidence, and Recommendation—is redacted first and then escaped for Markdown table safety, including pipes, newlines, backticks, and raw HTML. Apply the helper consistently to all interpolated values while preserving the displayed finding content and numeric line formatting.Source: Path instructions
examples/code_review_agent/internal/review/storage.go-94-96 (1)
94-96: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate JSON serialization failures before inserting rows.
Every
json.Marshalerror is discarded. If serialization fails, the transaction can commit an empty payload that normalized queries cannot decode. Marshal and validate all payloads before their corresponding inserts, returning the original error.中文
在插入数据库前传播 JSON 序列化错误。
所有
json.Marshal错误都被忽略。序列化失败时，事务可能提交无法被查询接口解码的空 payload。请在插入前完成并校验所有序列化操作，并返回原始错误。As per path instructions, persistence errors must preserve their root cause and must not silently corrupt audit records.
Also applies to: 110-128
🤖 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/code_review_agent/internal/review/storage.go` around lines 94 - 96, Update the report persistence flow around the SandboxRuns insert and the corresponding payload inserts near the referenced area to check each json.Marshal result before executing its SQL insert. If marshaling fails, return the original error immediately and do not insert that row; preserve the existing transaction error propagation and prevent any invalid or empty payload from being committed.Source: Path instructions
examples/code_review_agent/internal/review/sandbox.go-245-282 (1)
245-282: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftPrevent a symlink-swap escape while copying snapshots.
The file type is checked during
WalkDir, butcopyBoundedFilelater reopens the pathname normally. A concurrent replacement with a symlink can therefore copy a host file outside the reviewed repository into the sandbox. Open with no-follow semantics and validate the opened descriptor before copying.中文
防止快照复制期间通过替换符号链接越界。
文件类型在
WalkDir阶段检查，但copyBoundedFile随后会按路径重新打开文件。攻击者可在两者之间将文件替换为符号链接，使仓库外的宿主机文件被复制进沙箱。请使用禁止跟随链接的方式打开，并校验已打开的文件描述符。As per path instructions, repository paths are untrusted and must not cross the sandbox boundary through TOCTOU races.
Also applies to: 291-306
🤖 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/code_review_agent/internal/review/sandbox.go` around lines 245 - 282, Update copyBoundedFile and its call site in the filepath.WalkDir flow to open source files with no-follow semantics, preventing symlink traversal during the reopen. Validate the opened file descriptor’s metadata confirms it is a regular, non-symlink file before copying, and perform copying from that validated descriptor rather than reopening the untrusted pathname.Source: Path instructions
examples/code_review_agent/internal/review/sandbox.go-309-317 (1)
309-317: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not publish artifact metadata when the artifact write fails.
MkdirAllandWriteFileerrors are discarded, yet a successful-lookingArtifactis always returned. An unwritable output directory therefore creates a false audit record pointing to a missing or stale file. Propagate the failure and use the atomic writer.中文
产物写入失败时不要发布产物元数据。
MkdirAll和WriteFile的错误被忽略，但函数始终返回看似成功的Artifact。输出目录不可写时，审计记录会指向不存在或陈旧的文件。请传播错误并使用原子写入函数。As per path instructions, audit artifacts and failure records must remain consistent.
🤖 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/code_review_agent/internal/review/sandbox.go` around lines 309 - 317, Update sandbox.writeDiffStats to propagate MkdirAll, serialization, and artifact-write failures instead of returning successful metadata; adjust its result contract as needed so callers receive the error and no Artifact on failure. Replace the direct os.WriteFile call with the repository’s established atomic writer, while preserving the size limit and diff_stats.json metadata for successful writes.Source: Path instructions
examples/code_review_agent/internal/review/storage.go-51-64 (1)
51-64: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEnforce private permissions for the SQLite database and sidecars.
MkdirAlldoes not tighten an existing directory, while SQLite file creation remains umask-dependent. The normalized audit database can consequently be readable by other users. Create it in a private directory and explicitly enforce restrictive permissions for the database and journal files.中文
强制限制 SQLite 数据库及其辅助文件的权限。
MkdirAll不会收紧已有目录的权限，而 SQLite 文件创建权限依赖 umask。因此包含审计记录的数据库可能被其他用户读取。请使用私有目录，并明确限制数据库及日志文件权限。As per path instructions, persistence and sensitive audit boundaries must remain secure.
🤖 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/code_review_agent/internal/review/storage.go` around lines 51 - 64, Update openStore to enforce private permissions: explicitly chmod the database directory after MkdirAll, create/open the SQLite database with restrictive permissions, and chmod the database plus SQLite sidecar files such as journal/WAL files. Preserve cleanup on every failure, including closing db, and ensure the existing migrate flow only proceeds after permissions are secured.Source: Path instructions
examples/code_review_agent/internal/review/report.go-45-50 (1)
45-50: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPublish the JSON/Markdown pair without deleting the last good report.
atomicWriteremoves the target before renaming, creating an absence window and losing the previous report if rename fails. Additionally, JSON is committed before Markdown, so publication can leave a mismatched pair. Stage both outputs and commit them through a versioned directory or atomic manifest pointer.中文
发布 JSON/Markdown 文件对时，不要先删除上一份有效报告。
atomicWrite在重命名前删除目标，会产生文件缺失窗口；重命名失败时还会丢失旧报告。此外 JSON 先于 Markdown 提交，可能留下版本不一致的文件对。请先暂存两份输出，再通过版本目录或原子清单指针统一提交。As per path instructions, report publication must preserve audit consistency and stable externally observable behavior.
Also applies to: 59-85
🤖 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/code_review_agent/internal/review/report.go` around lines 45 - 50, Replace the sequential atomicWrite calls in the report publication flow with staged JSON and Markdown outputs committed as one versioned directory or via an atomic manifest pointer. Preserve the last complete report if staging or commit fails, and ensure readers observe only matching JSON/Markdown pairs; update the related publication logic covering the ReportPaths handling as well.Source: Path instructions
examples/code_review_agent/internal/review/sandbox.go-202-208 (1)
202-208: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact before truncation and cap every synthesized error.
Truncating first can leave secret fragments at the cutoff that no longer match redaction patterns. Appending
err.Error()afterward—and storing setup errors directly—also bypassesOutputLimit. Centralize normalization as redact-then-truncate for all recorded output, including setup failures.中文
先脱敏再截断，并限制所有合成错误的长度。
先截断可能在边界留下无法匹配脱敏规则的敏感片段。随后追加
err.Error()，以及直接保存初始化错误，也都会绕过OutputLimit。请统一采用“先脱敏、后截断”，并覆盖初始化失败。As per path instructions, sandbox output limits and sensitive-data boundaries must remain enforceable on every error path.
Also applies to: 320-321
🤖 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/code_review_agent/internal/review/sandbox.go` around lines 202 - 208, Update the sandbox output normalization around SandboxRun construction to redact each output before truncating it, and apply the same redact-then-truncate pipeline to synthesized execution errors. Ensure the combined stderr and err.Error() value is capped by outputLimit, and reuse this normalization for setup-failure paths around the referenced error handling so every recorded output respects sensitivity and length limits.Source: Path instructions
examples/code_review_agent/skills/code-review/scripts/diff_stats.sh-8-13 (1)
8-13: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCount changed lines only while inside hunks.
^+[^+]and^-[^-]omit empty additions/deletions and valid content beginning with++or--. Track@@hunk boundaries instead, then count every leading+/-line inside a hunk; headers occur outside hunks.中文
仅在 diff hunk 内统计变更行。
^+[^+]和^-[^-]会漏掉空白新增/删除行，以及以++或--开头的有效内容。请跟踪@@hunk 边界，并在 hunk 内统计所有以+/-开头的行；文件头位于 hunk 外。As per path instructions, example outputs must accurately reflect the current behavior.
🤖 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/code_review_agent/skills/code-review/scripts/diff_stats.sh` around lines 8 - 13, Update the diff line-counting logic in the script to track whether parsing is inside an @@ hunk, and count every line beginning with + or - only while inside a hunk. Preserve file counting, output formatting, and existing behavior for non-hunk headers while including empty additions/deletions and content beginning with ++ or --.Source: Path instructions
examples/code_review_agent/internal/review/sandbox.go-127-133 (1)
127-133: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the default container able to run
staticcheck. The sandbox still schedulesstaticcheckfor repo reviews, butexamples/code_review_agent/sandbox/Dockerfilenever installs it, so that check will always come back astool_unavailable. Install a pinned compatiblestaticcheckbinary in the image, or remove it from the default checklist.中文
让默认容器能够运行
staticcheck。 仓库审查仍会调度staticcheck，但examples/code_review_agent/sandbox/Dockerfile没有安装该二进制，因此这个检查会始终返回tool_unavailable。请在镜像中安装固定版本且兼容的staticcheck，或将其从默认检查列表中移除。🤖 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/code_review_agent/internal/review/sandbox.go` around lines 127 - 133, Ensure the default sandbox supports the staticcheck check: update examples/code_review_agent/sandbox/Dockerfile lines 1-4 to install a pinned compatible staticcheck binary, or remove the staticcheck entry from the checks list in examples/code_review_agent/internal/review/sandbox.go lines 127-133. Keep the default checklist and image configuration consistent so scheduled checks do not return tool_unavailable.Source: Path instructions
examples/code_review_agent/internal/review/review.go-66-84 (1)
66-84: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not publish a completed report before durable persistence succeeds.
TaskCompletedis set and report files are published before opening/saving SQLite. A database or cancellation failure therefore leaves authoritative-looking completed reports without the required audit record;EndedAtand total duration also exclude publication and persistence.Stage report files, persist the final report, then atomically expose them—or remove staged output and record failure when persistence fails.
中文
持久化成功前不要发布“已完成”报告。
代码先设置
TaskCompleted并发布报告文件，之后才打开并写入 SQLite。数据库或取消操作失败时，会遗留看似完整但缺少审计记录的报告；EndedAt和总耗时也未覆盖发布及持久化阶段。建议先暂存报告、持久化最终记录，再原子发布；失败时清理暂存输出并记录失败状态。
Based on the PR objectives requiring atomic report generation and persisted audit records.
🤖 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/code_review_agent/internal/review/review.go` around lines 66 - 84, Reorder the completion flow around report construction, publish, openStore, and store.Save so the report is staged rather than exposed, persistence succeeds before publication, and EndedAt plus metrics duration cover staging and database persistence. On any database, cancellation, or save failure, remove the staged output and record the failure state instead of leaving a completed report; only atomically expose the staged files after durable persistence succeeds.examples/code_review_agent/internal/review/review.go-202-206 (1)
202-206: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not require credential rotation for every critical finding.
Critical findings can cover SQL injection, lifecycle, concurrency, or other categories. The generic conclusion should require remediation only; add credential rotation conditionally for credential-related rules.
中文
不要要求所有严重问题都轮换凭据。
严重问题也可能属于 SQL 注入、生命周期或并发等类别。通用结论应只要求修复；仅对凭据相关规则附加凭据轮换要求。
Proposed fix
if finding.Severity == SeverityCritical { - return "Critical findings block merge until remediation and credential rotation are complete." + return "Critical findings block merge until remediation is complete." }As per path instructions, framework behavior should remain general rather than leak business-specific assumptions.
🤖 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/code_review_agent/internal/review/review.go` around lines 202 - 206, Update conclusion so SeverityCritical findings use a generic message requiring remediation only, and append the credential-rotation requirement only when the critical finding matches the credential-related rule/category. Keep conclusion’s behavior general for SQL injection, lifecycle, concurrency, and other non-credential findings.Source: Path instructions
examples/code_review_agent/internal/review/review.go-64-65 (1)
64-65: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDeduplicate findings before recording routing decisions.
The current order can retain multiple
route_humanaudit decisions for a fingerprint that appears only once inNeedsHumanReview. Deduplicate first so persisted decisions match the normalized final bucket.中文
请在记录路由决策前去重。
当前顺序可能为同一指纹保留多条
route_human审计记录，而最终NeedsHumanReview中只剩一条。应先去重，确保持久化决策与规范化后的结果一致。Proposed fix
-filterAudit = append(retainedAudit, filterDecisions(needsHuman, "needs_human_review", FilterRouteHuman)...) needsHuman = dedupe(needsHuman) +filterAudit = append(retainedAudit, filterDecisions(needsHuman, "needs_human_review", FilterRouteHuman)...)Based on the PR objective requiring finding deduplication and normalized audit records.
🤖 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/code_review_agent/internal/review/review.go` around lines 64 - 65, In the flow around filterAudit and needsHuman, deduplicate needsHuman before calling filterDecisions and appending the routing audit records. Then record decisions from the normalized deduplicated bucket so persisted route_human entries match the final NeedsHumanReview contents.
🟡 Minor comments (3)
examples/code_review_agent/internal/review/input.go-38-50 (1)
38-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize input modes consistently before validation and dispatch.
modesignores whitespace-only values, but the switch does not. For example,DiffFile: " "plus a valid fixture counts as one mode and then tries to read" "instead of the fixture. Dispatch using the same trimmed values.中文
请在校验和分派前统一规范化输入模式。
modes会忽略仅含空白的值，但后续switch不会。例如，DiffFile: " "与有效 fixture 同时存在时会被计为单一模式，却最终尝试读取" "。请使用同一组TrimSpace后的值进行分派。As per path instructions, framework defaults and input behavior must remain stable and predictable.
🤖 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/code_review_agent/internal/review/input.go` around lines 38 - 50, Normalize cfg.DiffFile, primaryRepo, cfg.FileList, and cfg.Fixture with strings.TrimSpace once before counting modes and dispatching in the input parsing flow. Update the switch and subsequent reads to use these normalized values, preserving the exactly-one-mode validation and existing behavior for non-whitespace inputs.Source: Path instructions
examples/code_review_agent/internal/review/review.go-180-184 (1)
180-184: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not classify an expected dry run as an error.
Every dry run records
error_type=dry_run, so successful deterministic executions produce a non-zero error distribution, as demonstrated by the sample report. Exclude expected control-flow outcomes from error metrics.中文
不要将预期的 dry-run 计为错误。
每次 dry-run 都会记录
error_type=dry_run，导致成功的确定性执行仍产生非零错误统计，示例报告也体现了该问题。错误指标应排除预期的控制流结果。🤖 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/code_review_agent/internal/review/review.go` around lines 180 - 184, Update the error classification logic in the SandboxRuns aggregation loop so runs with ErrorType equal to the expected dry-run outcome are excluded from metrics.ErrorDistribution. Continue counting all other non-empty error types and preserve the existing duration accumulation.examples/code_review_agent/internal/review/review.go-86-88 (1)
86-88: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve the storage verification error.
The combined condition replaces database/decode errors with a generic message, preventing callers from distinguishing a failed load from an ID mismatch.
中文
请保留存储校验的原始错误。
当前组合条件会用通用错误覆盖数据库或解码错误，使调用方无法区分加载失败和任务 ID 不匹配。
Proposed fix
stored, err := store.Load(ctx, task.ID) -if err != nil || stored.Task.ID != task.ID { - return Report{}, ReportPaths{}, errors.New("stored review verification failed") +if err != nil { + return Report{}, ReportPaths{}, fmt.Errorf("verify stored review: %w", err) +} +if stored.Task.ID != task.ID { + return Report{}, ReportPaths{}, fmt.Errorf( + "stored review task ID mismatch: got %q, want %q", + stored.Task.ID, task.ID, + ) }As per path instructions, errors must preserve root causes and remain distinguishable to callers.
🤖 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/code_review_agent/internal/review/review.go` around lines 86 - 88, Update the stored review verification around store.Load so loading or decoding errors return the original err unchanged, while a successfully loaded record with a mismatched Task.ID returns a separate verification error. Keep these two failure cases distinct for callers.Source: Path instructions
🧹 Nitpick comments (1)
examples/code_review_agent/internal/review/redact.go (1)
35-41: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEnsure UTF-8 safe string truncation.
Slicing strings by bytes (
value[:limit]) can cut multi-byte characters in half, resulting in invalid UTF-8 which may cause issues during JSON serialization or reporting. It's safer to truncate by runes.中文
按字节截断字符串（`value[:limit]`）可能会将多字节字符截断，从而产生无效的 UTF-8，这可能会在 JSON 序列化或生成报告时引发问题。建议按 `rune`（字符）进行截断以保证安全。♻️ Proposed refactor
func truncate(value string, limit int) (string, bool) { value = redact(value) if limit <= 0 || len(value) <= limit { return value, false } - return value[:limit] + "\n...[truncated]", true + runes := []rune(value) + if len(runes) <= limit { + return value, false + } + return string(runes[:limit]) + "\n...[truncated]", 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/code_review_agent/internal/review/redact.go` around lines 35 - 41, Update truncate to perform the limit comparison and prefix truncation by runes rather than byte indices, while preserving the existing return values and truncation marker. Ensure the resulting string remains valid UTF-8 for multibyte characters.
🤖 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.
Major comments:
In `@examples/code_review_agent/go.mod`:
- Line 25: Update the dependency declarations in the example module’s go.mod for
github.com/docker/docker, go.opentelemetry.io/otel/sdk, and
google.golang.org/grpc to patched releases; if any are only transitive, remove
their explicit requirements and regenerate the module graph accordingly.
In `@examples/code_review_agent/internal/review/input.go`:
- Around line 73-82: Update readBounded and the related command-output and
file-list reading paths to enforce maxInputBytes during streaming rather than
after full buffering. Replace os.ReadFile and cmd.Output usage with limited
readers or bounded pipes that detect and reject maxInputBytes+1 bytes, including
the file-list itself, while preserving existing error handling and returned
content for inputs within the limit.
- Around line 161-167: Update the synthetic diff generation around the hunk
header and line-emission loop so the declared added-line count exactly matches
the lines written. Handle newline-terminated contents without counting the
trimmed trailing line, and represent empty files with zero added lines rather
than an added blank line; preserve correct file-list diff semantics.
- Around line 203-218: Update the diff-header parsing in the input-processing
loop so `diff --git` paths are not split with `strings.Fields`, which truncates
filenames containing spaces and registers ghost entries. Use quoted/path-aware
parsing, or defer adding entries from `headerFile` until authoritative
`---`/`+++` headers; preserve accurate `files` registration and review locations
for all valid paths.
In `@examples/code_review_agent/internal/review/report.go`:
- Around line 124-137: Update escapeCell and renderFindingTable so every
untrusted table field—Severity, Category, File, Line-derived location, Title,
Evidence, and Recommendation—is redacted first and then escaped for Markdown
table safety, including pipes, newlines, backticks, and raw HTML. Apply the
helper consistently to all interpolated values while preserving the displayed
finding content and numeric line formatting.
- Around line 45-50: Replace the sequential atomicWrite calls in the report
publication flow with staged JSON and Markdown outputs committed as one
versioned directory or via an atomic manifest pointer. Preserve the last
complete report if staging or commit fails, and ensure readers observe only
matching JSON/Markdown pairs; update the related publication logic covering the
ReportPaths handling as well.
In `@examples/code_review_agent/internal/review/review.go`:
- Around line 66-84: Reorder the completion flow around report construction,
publish, openStore, and store.Save so the report is staged rather than exposed,
persistence succeeds before publication, and EndedAt plus metrics duration cover
staging and database persistence. On any database, cancellation, or save
failure, remove the staged output and record the failure state instead of
leaving a completed report; only atomically expose the staged files after
durable persistence succeeds.
- Around line 202-206: Update conclusion so SeverityCritical findings use a
generic message requiring remediation only, and append the credential-rotation
requirement only when the critical finding matches the credential-related
rule/category. Keep conclusion’s behavior general for SQL injection, lifecycle,
concurrency, and other non-credential findings.
- Around line 64-65: In the flow around filterAudit and needsHuman, deduplicate
needsHuman before calling filterDecisions and appending the routing audit
records. Then record decisions from the normalized deduplicated bucket so
persisted route_human entries match the final NeedsHumanReview contents.
In `@examples/code_review_agent/internal/review/rules_test.go`:
- Around line 50-53: Update TestCleanFixtureHasNoObservation and the
additionally affected fixture-loading tests to check and fail on errors returned
by exampleDir and os.ReadFile before parsing the fixture. Preserve the existing
ParseUnifiedDiff assertions, ensuring missing or unreadable fixtures cannot be
treated as empty input.
In `@examples/code_review_agent/internal/review/rules.go`:
- Around line 223-233: Update missingTests to associate changed tests with
production Go files by package or directory rather than using any
repository-wide _test.go file. Preserve each file’s change status so deleted
tests are excluded from coverage, and ensure unrelated test changes cannot
suppress findings for production files without matching active tests.
- Line 32: Restrict the dynamicShell rule in the rules configuration to report
only dynamically constructed command strings passed to sh -c or bash -c,
removing the broad + and fmt.Sprintf alternatives that match safe direct
execution. Add a negative test covering direct exec.Command arguments such as
git show with a dynamic path, and retain positive coverage for dynamically built
shell command strings.
- Around line 208-220: Update findingLocationKey to include Finding.RuleID in
the deduplication key, while preserving the existing file, line, and category
components. Keep selectBestFindings’ confidence-based selection unchanged so
only repeated instances of the same rule are deduplicated.
In `@examples/code_review_agent/internal/review/sandbox.go`:
- Around line 158-162: Update the workspace cleanup defer around
Manager().Cleanup to use a fresh context with a short timeout rather than
context.WithoutCancel(ctx), ensuring cleanup cannot delay run indefinitely.
Capture and record any Cleanup failure, and release the timeout context after
cleanup completes.
- Around line 209-217: Update the staticcheck classification in the
result-processing flow so “tool_unavailable” is assigned only for an explicit
command-not-found result when the run has not timed out or failed at the
executor layer; preserve timeout and executor-error statuses and error types.
Extend TestExecuteClassifiesResults with combined staticcheck timeout and
executor-error cases, ensuring sandbox errors remain distinguishable.
- Around line 245-282: Update copyBoundedFile and its call site in the
filepath.WalkDir flow to open source files with no-follow semantics, preventing
symlink traversal during the reopen. Validate the opened file descriptor’s
metadata confirms it is a regular, non-symlink file before copying, and perform
copying from that validated descriptor rather than reopening the untrusted
pathname.
- Around line 309-317: Update sandbox.writeDiffStats to propagate MkdirAll,
serialization, and artifact-write failures instead of returning successful
metadata; adjust its result contract as needed so callers receive the error and
no Artifact on failure. Replace the direct os.WriteFile call with the
repository’s established atomic writer, while preserving the size limit and
diff_stats.json metadata for successful writes.
- Around line 202-208: Update the sandbox output normalization around SandboxRun
construction to redact each output before truncating it, and apply the same
redact-then-truncate pipeline to synthesized execution errors. Ensure the
combined stderr and err.Error() value is capped by outputLimit, and reuse this
normalization for setup-failure paths around the referenced error handling so
every recorded output respects sensitivity and length limits.
- Around line 127-133: Ensure the default sandbox supports the staticcheck
check: update examples/code_review_agent/sandbox/Dockerfile lines 1-4 to install
a pinned compatible staticcheck binary, or remove the staticcheck entry from the
checks list in examples/code_review_agent/internal/review/sandbox.go lines
127-133. Keep the default checklist and image configuration consistent so
scheduled checks do not return tool_unavailable.
In `@examples/code_review_agent/internal/review/storage.go`:
- Around line 94-96: Update the report persistence flow around the SandboxRuns
insert and the corresponding payload inserts near the referenced area to check
each json.Marshal result before executing its SQL insert. If marshaling fails,
return the original error immediately and do not insert that row; preserve the
existing transaction error propagation and prevent any invalid or empty payload
from being committed.
- Around line 51-64: Update openStore to enforce private permissions: explicitly
chmod the database directory after MkdirAll, create/open the SQLite database
with restrictive permissions, and chmod the database plus SQLite sidecar files
such as journal/WAL files. Preserve cleanup on every failure, including closing
db, and ensure the existing migrate flow only proceeds after permissions are
secured.
In `@examples/code_review_agent/scripts/run_fixtures.sh`:
- Around line 8-19: Update the fixture loop in run_fixtures.sh to isolate each
invocation from prior output by cleaning or using a unique per-run directory for
each fixture. Replace the find/head report discovery with the exact
review_report.json path produced by that fixture run, preserving the existing jq
summary behavior and avoiding pipefail issues.
In `@examples/code_review_agent/skills/code-review/scripts/diff_stats.sh`:
- Around line 8-13: Update the diff line-counting logic in the script to track
whether parsing is inside an @@ hunk, and count every line beginning with + or -
only while inside a hunk. Preserve file counting, output formatting, and
existing behavior for non-hunk headers while including empty additions/deletions
and content beginning with ++ or --.
---
Minor comments:
In `@examples/code_review_agent/internal/review/input.go`:
- Around line 38-50: Normalize cfg.DiffFile, primaryRepo, cfg.FileList, and
cfg.Fixture with strings.TrimSpace once before counting modes and dispatching in
the input parsing flow. Update the switch and subsequent reads to use these
normalized values, preserving the exactly-one-mode validation and existing
behavior for non-whitespace inputs.
In `@examples/code_review_agent/internal/review/review.go`:
- Around line 180-184: Update the error classification logic in the SandboxRuns
aggregation loop so runs with ErrorType equal to the expected dry-run outcome
are excluded from metrics.ErrorDistribution. Continue counting all other
non-empty error types and preserve the existing duration accumulation.
- Around line 86-88: Update the stored review verification around store.Load so
loading or decoding errors return the original err unchanged, while a
successfully loaded record with a mismatched Task.ID returns a separate
verification error. Keep these two failure cases distinct for callers.
---
Nitpick comments:
In `@examples/code_review_agent/internal/review/redact.go`:
- Around line 35-41: Update truncate to perform the limit comparison and prefix
truncation by runes rather than byte indices, while preserving the existing
return values and truncation marker. Ensure the resulting string remains valid
UTF-8 for multibyte characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70ac85ec-8b24-4c6c-b506-a7643714b450
⛔ Files ignored due to path filters (1)
examples/code_review_agent/go.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
examples/code_review_agent/.gitignoreexamples/code_review_agent/DESIGN.mdexamples/code_review_agent/README.mdexamples/code_review_agent/fixtures/clean.diffexamples/code_review_agent/fixtures/context.diffexamples/code_review_agent/fixtures/database.diffexamples/code_review_agent/fixtures/duplicate.diffexamples/code_review_agent/fixtures/errors.diffexamples/code_review_agent/fixtures/goroutine.diffexamples/code_review_agent/fixtures/missing_test.diffexamples/code_review_agent/fixtures/resource.diffexamples/code_review_agent/fixtures/sandbox_failure.diffexamples/code_review_agent/fixtures/secret.diffexamples/code_review_agent/fixtures/sql_injection.diffexamples/code_review_agent/go.modexamples/code_review_agent/internal/review/coverage_test.goexamples/code_review_agent/internal/review/governance_test.goexamples/code_review_agent/internal/review/input.goexamples/code_review_agent/internal/review/input_test.goexamples/code_review_agent/internal/review/pipeline_test.goexamples/code_review_agent/internal/review/policy.goexamples/code_review_agent/internal/review/redact.goexamples/code_review_agent/internal/review/report.goexamples/code_review_agent/internal/review/review.goexamples/code_review_agent/internal/review/rules.goexamples/code_review_agent/internal/review/rules_test.goexamples/code_review_agent/internal/review/sandbox.goexamples/code_review_agent/internal/review/storage.goexamples/code_review_agent/internal/review/types.goexamples/code_review_agent/main.goexamples/code_review_agent/sample_output/sample-secret/diff_stats.jsonexamples/code_review_agent/sample_output/sample-secret/review_report.jsonexamples/code_review_agent/sample_output/sample-secret/review_report.mdexamples/code_review_agent/sandbox/Dockerfileexamples/code_review_agent/scripts/run_fixtures.ps1examples/code_review_agent/scripts/run_fixtures.shexamples/code_review_agent/skills/code-review/SKILL.mdexamples/code_review_agent/skills/code-review/docs/go-review-rules.mdexamples/code_review_agent/skills/code-review/scripts/diff_stats.sh
42ea4c1 to
ac44fda
Compare
Review Feedback UpdateThis commit ( It hardens input and diff parsing, improves rule accuracy, strengthens sandbox isolation and cleanup, provides safer dependency handling, and makes report publication atomic. It also improves Markdown escaping, secret redaction, SQLite storage security, fixture scripts, and Docker tooling. Additional regression tests cover paths containing spaces, file renames, binary files, multiline shell commands, UTF-8 truncation, report publication failures, and sandbox errors. All unit tests, race tests, fixture scenarios, |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/code_review_agent/internal/review/sandbox.go (1)
359-377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNormalize artifact paths with
filepath.ToSlashfor cross-platform consistency.Using
filepath.Joinfor artifact paths results in OS-specific separators (e.g.,\on Windows) leaking into the JSON reports, as seen in the sample output. Wrapping the path infilepath.ToSlashensures deterministic, OS-agnostic outputs across different execution environments.中文
**使用 `filepath.ToSlash` 规范化工件路径以保持跨平台一致性。**使用
filepath.Join生成工件路径会导致特定于操作系统的分隔符（例如 Windows 上的\）泄露到 JSON 报告中，这在示例输出中可见。使用filepath.ToSlash包装路径可确保在不同执行环境下得到确定的、与操作系统无关的输出。♻️ Proposed fix
if err := atomicWrite(file, data); err != nil { return Artifact{}, err } - return Artifact{Name: "diff_stats.json", Path: file, MIMEType: "application/json", SizeBytes: int64(len(data))}, nil + return Artifact{Name: "diff_stats.json", Path: filepath.ToSlash(file), MIMEType: "application/json", SizeBytes: int64(len(data))}, nil }🤖 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/code_review_agent/internal/review/sandbox.go` around lines 359 - 377, Normalize the artifact path in writeDiffStats by applying filepath.ToSlash to the filepath.Join result before storing or returning it, ensuring diff statistics reports use deterministic slash separators across operating systems.
🤖 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/code_review_agent/internal/review/sandbox.go`:
- Around line 56-58: Set the container’s working directory to the writable
reviewer home in both affected sites: update containerexec.WithContainerConfig
in examples/code_review_agent/internal/review/sandbox.go lines 56-58 to use
"/home/reviewer", and change WORKDIR in
examples/code_review_agent/sandbox/Dockerfile line 10 from "/" to
"/home/reviewer".
In `@examples/code_review_agent/internal/review/storage.go`:
- Around line 58-60: Remove the unconditional os.Chmod call from the directory
setup flow in storage.go. Keep filepath.Dir and os.MkdirAll so newly created
directories receive the intended permissions while existing directories,
including ".", remain unchanged.
---
Nitpick comments:
In `@examples/code_review_agent/internal/review/sandbox.go`:
- Around line 359-377: Normalize the artifact path in writeDiffStats by applying
filepath.ToSlash to the filepath.Join result before storing or returning it,
ensuring diff statistics reports use deterministic slash separators across
operating systems.
🪄 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: 551c512c-5d3d-4f28-9ec3-6c8e157c2d74
⛔ Files ignored due to path filters (1)
examples/code_review_agent/go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
examples/code_review_agent/README.mdexamples/code_review_agent/go.modexamples/code_review_agent/internal/review/coverage_test.goexamples/code_review_agent/internal/review/governance_test.goexamples/code_review_agent/internal/review/input.goexamples/code_review_agent/internal/review/input_test.goexamples/code_review_agent/internal/review/pipeline_test.goexamples/code_review_agent/internal/review/redact.goexamples/code_review_agent/internal/review/report.goexamples/code_review_agent/internal/review/review.goexamples/code_review_agent/internal/review/rules.goexamples/code_review_agent/internal/review/rules_test.goexamples/code_review_agent/internal/review/sandbox.goexamples/code_review_agent/internal/review/storage.goexamples/code_review_agent/internal/review/types.goexamples/code_review_agent/sample_output/sample-secret/report/review_report.jsonexamples/code_review_agent/sample_output/sample-secret/report/review_report.mdexamples/code_review_agent/sandbox/Dockerfileexamples/code_review_agent/scripts/run_fixtures.ps1examples/code_review_agent/scripts/run_fixtures.shexamples/code_review_agent/skills/code-review/scripts/diff_stats.sh
🚧 Files skipped from review as they are similar to previous changes (12)
- examples/code_review_agent/skills/code-review/scripts/diff_stats.sh
- examples/code_review_agent/README.md
- examples/code_review_agent/scripts/run_fixtures.sh
- examples/code_review_agent/go.mod
- examples/code_review_agent/internal/review/governance_test.go
- examples/code_review_agent/scripts/run_fixtures.ps1
- examples/code_review_agent/internal/review/types.go
- examples/code_review_agent/internal/review/report.go
- examples/code_review_agent/internal/review/redact.go
- examples/code_review_agent/internal/review/review.go
- examples/code_review_agent/internal/review/rules.go
- examples/code_review_agent/internal/review/input.go
| containerexec.WithContainerConfig(dockercontainer.Config{ | ||
| Image: containerImageTag, WorkingDir: "/", Cmd: []string{"tail", "-f", "/dev/null"}, Tty: true, OpenStdin: true, | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent permission denied errors by setting a writable workspace directory for the unprivileged user.
The container image creates an unprivileged reviewer user but sets the working directory to /. Because / is owned by root, the container executor will fail with a permission denied error when it attempts to create the execution workspace directory during setup.
examples/code_review_agent/internal/review/sandbox.go#L56-L58: UpdateWorkingDirto"/home/reviewer"in the container configuration.examples/code_review_agent/sandbox/Dockerfile#L10-L10: ChangeWORKDIR /toWORKDIR /home/reviewer.
中文
**为非特权用户设置可写的运行目录以防止权限被拒绝。**容器镜像创建了非特权用户 reviewer，但将工作目录设置为了 /。由于 / 的所有者是 root，在设置阶段容器执行器试图创建执行工作区目录时，将因权限不足而失败。
examples/code_review_agent/internal/review/sandbox.go#L56-L58: 在容器配置中将WorkingDir更新为"/home/reviewer"。examples/code_review_agent/sandbox/Dockerfile#L10-L10: 将WORKDIR /更改为WORKDIR /home/reviewer"。
📍 Affects 2 files
examples/code_review_agent/internal/review/sandbox.go#L56-L58(this comment)examples/code_review_agent/sandbox/Dockerfile#L10-L10
🤖 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/code_review_agent/internal/review/sandbox.go` around lines 56 - 58,
Set the container’s working directory to the writable reviewer home in both
affected sites: update containerexec.WithContainerConfig in
examples/code_review_agent/internal/review/sandbox.go lines 56-58 to use
"/home/reviewer", and change WORKDIR in
examples/code_review_agent/sandbox/Dockerfile line 10 from "/" to
"/home/reviewer".
| if err := os.Chmod(dir, 0o700); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove unconditional directory permission changes.
If the provided DB path is a flat filename (e.g., "agent.db"), filepath.Dir(path) evaluates to ".". Running os.Chmod(".", 0o700) will unexpectedly change the permissions of the current working directory, which might break workspace access for other users or tools. os.MkdirAll already safely applies 0700 to newly created directories while leaving existing directory permissions intact.
中文
移除无条件的目录权限修改
如果提供的 DB path 是一个纯文件名（例如 "agent.db"），filepath.Dir(path) 会被计算为 "."。执行 os.Chmod(".", 0o700) 会意外地修改当前工作目录的权限，这可能会破坏其他用户或工具对该工作区的访问。os.MkdirAll 已经能够安全地为新创建的目录应用 0700 权限，同时保持已存在目录的权限不变，因此无需再额外调用 Chmod。
♻️ Proposed fix
if err := os.MkdirAll(dir, 0o700); err != nil {
return nil, err
}
- if err := os.Chmod(dir, 0o700); err != nil {
- return nil, err
- }
_, statErr := os.Stat(path)📝 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.
| if err := os.Chmod(dir, 0o700); err != nil { | |
| return nil, err | |
| } |
| if err := os.Chmod(dir, 0o700); err != nil { | |
| return nil, err | |
| } |
🤖 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/code_review_agent/internal/review/storage.go` around lines 58 - 60,
Remove the unconditional os.Chmod call from the directory setup flow in
storage.go. Keep filepath.Dir and os.MkdirAll so newly created directories
receive the intended permissions while existing directories, including ".",
remain unchanged.
Rememorio
left a comment
Rememorio
left a comment
There was a problem hiding this comment.
I reviewed the changed lines and left 7 inline comments below. The comments focus on issues worth addressing before merge.
Fresh review found seven high-confidence issues. The main risks are false critical secret findings, ineffective runtime output limits, missing container resource ceilings, mutation of existing directory permissions, and audit corruption when task IDs are reused. Targeted regression tests should cover safe environment-based secret loading, oversized command output, container limits, duplicate task IDs, existing database-directory modes, and rename-only diffs.
中文
我看过这次变更的相关代码，在下面留下 7 条行内评论。评论聚焦在合并前值得处理的问题。
本次审查发现 7 个高置信度问题。主要风险包括：安全的密钥加载方式被误判为严重问题、运行时输出限制实际无效、容器缺少资源上限、数据库初始化会修改已有目录权限，以及复用 task ID 时破坏审计制品一致性。建议补充针对环境变量密钥读取、超量命令输出、容器资源限制、重复 task ID、已有数据库目录权限和纯重命名 diff 的回归测试。
| func looksSecret(value string) bool { | ||
| lower := strings.ToLower(value) | ||
| for _, key := range []string{"password", "passwd", "token", "api_key", "apikey", "secret", "private_key"} { | ||
| if strings.Contains(lower, key) && strings.ContainsAny(value, `="':`) { |
There was a problem hiding this comment.
P1: Distinguish secret names from literal secret values
This condition also matches safe code such as token := os.Getenv("TOKEN") and fields such as Token string json:"token"`` because it only requires a keyword and punctuation. Each match becomes a critical finding and a merge-blocking conclusion, so recommended secret-loading patterns produce false blockers.
Require evidence of a literal credential value, or parse the assignment sufficiently to exclude environment and secret-manager reads. Add negative assertions for environment-variable access and credential-named struct fields.
中文
区分密钥名称与实际硬编码值
这里仅凭密钥关键词和标点就判定为硬编码密钥，因此 token := os.Getenv("TOKEN") 以及带有 json:"token" 标签的 Token 字段也会命中。该规则会生成 critical finding，并直接给出阻止合并的结论，导致推荐的安全加载方式被误报。
建议只在确认存在字面量凭据值时命中，或解析赋值表达式并排除环境变量、密钥管理器等来源；同时为环境变量读取和密钥命名字段补充负向测试。
| var containerFactory engineFactory = func(_ context.Context, _ Config, baseDir string) (codeexecutor.Engine, func() error, error) { | ||
| executor, err := containerexec.New( | ||
| containerexec.WithDockerFilePath(filepath.Join(baseDir, "sandbox")), | ||
| containerexec.WithContainerConfig(dockercontainer.Config{ |
There was a problem hiding this comment.
P1: Bound container resources before running repository tests
The default backend executes repository-controlled go test code, but this configuration leaves the inherited Docker host settings without memory, CPU, or PID ceilings. A malicious or broken test can fork processes or allocate until the host is exhausted, independently of the command timeout and output truncation.
Pass an explicit host configuration that preserves auto-removal and NetworkMode: none while adding memory, CPU, and PID limits, plus any available writable-storage bound. Validate those limits in the container configuration test.
中文
运行仓库测试前应限制容器资源
默认后端会执行仓库控制的 go test 代码，但当前 Docker host 配置没有设置内存、CPU 或进程数上限。恶意或异常测试可以大量创建进程或持续分配资源，直到宿主机耗尽；命令超时和输出截断都无法防止这种情况。
建议显式传入 host 配置，在保留自动清理和 NetworkMode: none 的同时设置内存、CPU、PID 以及可用的可写存储限制，并在容器配置测试中验证这些约束。
| result, err := s.engine.Runner().RunProgram(ctx, ws, codeexecutor.RunProgramSpec{ | ||
| Cmd: command, Args: args, Cwd: cwd, Timeout: s.timeout, CleanEnv: true, Env: reviewEnvironment(s.executor), | ||
| }) | ||
| stdout, stdoutCut := truncate(result.Stdout, s.outputLimit) |
There was a problem hiding this comment.
P1: Enforce the output limit while the command is running
OutputLimit is applied only after RunProgram returns. The container, local, and E2B runners accumulate stdout and stderr in memory first, so an untrusted test that continuously prints can consume unbounded memory during the command timeout; OutputTruncated is set too late to protect the process.
Enforce a cap in the execution path using bounded streaming writers or cancellation after the cap is reached. A regression test should produce more than the configured limit and verify that the full output is never retained in memory.
中文
应在命令运行过程中限制输出
OutputLimit 只在 RunProgram 返回后才生效，而 container、local 和 E2B runner 都会先把 stdout、stderr 完整累积到内存中。恶意或异常测试持续输出时，可在超时前占用无上限的内存；事后设置 OutputTruncated 无法保护运行进程。
应在执行过程中通过有界流式写入，或在达到上限后取消命令来真正限流。建议增加超量输出测试，并验证执行层不会保留完整输出。
| if len(data) > maxArtifactBytes { | ||
| return Artifact{}, errors.New("diff statistics exceed artifact limit") | ||
| } | ||
| if err := atomicWrite(file, data); err != nil { |
There was a problem hiding this comment.
P1: Reject duplicate task IDs before overwriting artifacts
diff_stats.json is overwritten before Store.Save or the staged report commit detects that the task ID already exists. Re-running the same ID with different input therefore replaces the old statistics and then returns an error, leaving the existing database/report paired with a new artifact.
Reserve the task ID before writing, or stage all artifacts and publish them with no-clobber semantics after persistence succeeds. A two-run regression should verify that a rejected duplicate leaves every artifact from the first run unchanged.
中文
覆盖制品前应先拒绝重复 task ID
当前会先覆盖 diff_stats.json，之后 Store.Save 或报告提交阶段才发现 task ID 已存在。使用不同输入复用同一 ID 时，第二次运行虽然最终报错，却已经替换了旧统计文件，导致已有数据库和报告对应到新的制品。
建议在写入前预留 task ID，或统一暂存所有制品，并在持久化成功后以禁止覆盖的方式发布。应增加连续运行两次的回归测试，确认被拒绝的重复任务不会修改第一次运行的任何制品。
| if err := os.MkdirAll(dir, 0o700); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := os.Chmod(dir, 0o700); err != nil { |
There was a problem hiding this comment.
P1: Do not chmod an existing database parent directory
openStore unconditionally changes the database parent to mode 0700. For example, --db ./reviews.sqlite changes the current directory, while a database in an owned shared directory removes access for other users and tools.
Track whether this call created the directory and only apply the private mode in that case; the database file itself can still be chmodded to 0600. Add a test proving that an existing parent's mode remains unchanged.
中文
不要修改已有数据库父目录的权限
openStore 会无条件把数据库父目录改为 0700。例如使用 --db ./reviews.sqlite 会修改当前目录权限；若数据库位于用户拥有的共享目录中，还会移除其他用户和工具的访问权限。
建议记录目录是否由本次调用创建，仅对新建目录设置私有权限；数据库文件本身仍可设置为 0600。同时补充测试，确保已有父目录的权限保持不变。
| headerFile = diffHeaderTarget(line) | ||
| inHunk = false | ||
| case strings.HasPrefix(line, "rename to "): | ||
| currentFile = cleanDiffPath(strings.TrimPrefix(line, "rename to ")) |
There was a problem hiding this comment.
P2: Preserve leading directories in rename metadata
A rename to value is already repository-relative, but cleanDiffPath removes a leading a/ or b/ as though it were a ---/+++ header. A pure rename to a/new.go, for example, is recorded as new.go, which changes its package and can misroute test-coverage decisions.
Use separate normalization for repository-relative rename paths and prefixed diff-header paths. Add pure-rename fixtures targeting top-level a/ and b/ directories.
中文
纯重命名时应保留开头目录
rename to 后的路径本身已经是仓库相对路径，但 cleanDiffPath 仍会像处理 ---、+++ 头部一样移除开头的 a/ 或 b/。例如纯重命名到 a/new.go 会被记录成 new.go，从而改变包目录并影响测试覆盖路由。
建议分别处理仓库相对的重命名路径和带 diff 前缀的头部路径，并增加目标位于顶层 a/、b/ 目录的纯重命名测试。
|
|
||
| type engineFactory func(context.Context, Config, string) (codeexecutor.Engine, func() error, error) | ||
|
|
||
| var containerFactory engineFactory = func(_ context.Context, _ Config, baseDir string) (codeexecutor.Engine, func() error, error) { |
There was a problem hiding this comment.
P2: Propagate cancellation through container initialization
The factory accepts the caller context but discards it, while containerexec.New performs image build, pull, and container creation with a background context. Cancellation or SIGTERM therefore cannot stop default-backend setup, and Run may remain blocked well beyond the configured command timeout.
Use a context-aware container constructor or extend the container executor to accept the setup context. Add a cancelled-context test that verifies initialization returns promptly.
中文
容器初始化应传播取消信号
工厂函数接收了调用方 context 却直接丢弃，而 containerexec.New 使用后台 context 执行镜像构建、拉取和容器创建。因此取消请求或 SIGTERM 无法中止默认后端初始化，Run 可能远超命令超时时间仍然阻塞。
建议使用支持 context 的容器构造接口，或扩展 container executor 让初始化接收调用方 context；同时增加已取消 context 的测试，验证初始化能够及时返回。
3 participants
Summary
This PR adds an auditable Go code-review agent example that addresses #2004.
The example loads a repository-native review Skill, analyzes bounded Go diffs, executes approved checks in an isolated sandbox, persists normalized audit records in SQLite, and publishes redacted JSON and Markdown reports.
What changed
PermissionPolicyfor sandbox commands.Safety and auditability
Verification
go test -count=1 ./...go vet ./...go test -race ./internal/review85.5% of statements86.4%git diff --check main...HEADpassed.A real Docker smoke run could not be completed locally because the Docker daemon was unavailable. Container configuration, Dockerfile availability, capability checks, and executor initialization failure handling are covered by automated tests.
Fixes #2004