新增 Go 自动代码评审 Agent 示例#2240
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2240 +/- ##
===================================================
- Coverage 89.88607% 89.85483% -0.03124%
===================================================
Files 1144 1150 +6
Lines 198637 203417 +4780
===================================================
+ Hits 178547 182780 +4233
- Misses 12588 12919 +331
- Partials 7502 7718 +216
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:
|
|
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:
📝 WalkthroughWalkthroughThe PR adds a runnable Go code review agent with configurable inputs, deterministic rules, optional sandbox and model checks, redacted findings, report generation, SQLite persistence, telemetry, CLI configuration, and fixture-based validation. ChangesCode Review Agent
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
examples/code_review_agent/internal/review/types.go-267-270 (1)
267-270: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
MustJSONsilently swallows marshal errors, contrary to its "Must" naming.
Must*helpers conventionally panic on failure; this one discards the error and returns whatever partial/empty bytesjson.MarshalIndentproduced. IfResultever contains something unmarshalable, callers writing report artifacts would silently persist empty/corrupt JSON with no signal.🐛 Proposed fix
func MustJSON(v any) []byte { - b, _ := json.MarshalIndent(v, "", " ") + b, err := json.MarshalIndent(v, "", " ") + if err != nil { + panic(err) + } return b }🤖 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/types.go` around lines 267 - 270, Update MustJSON to handle the error returned by json.MarshalIndent instead of discarding it; preserve successful output, but panic with the marshal error when serialization fails so the helper’s Must contract is enforced.examples/code_review_agent/testdata/holdout/model-semantic.diff-9-11 (1)
9-11: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake semantic holdouts behaviorally reproduce their marker claims.
Both fixtures currently test marker recognition rather than semantic review:
examples/code_review_agent/testdata/holdout/model-semantic.diff#L9-L11: connect the new owner bypass to a previously restrictive authorization decision.examples/code_review_agent/testdata/holdout/model-transaction-semantic.diff#L8-L11: mutate shared or persisted status so commit failure leaves observable inconsistent state.🤖 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/testdata/holdout/model-semantic.diff` around lines 9 - 11, Make the semantic holdouts reproduce their claimed behaviors: in examples/code_review_agent/testdata/holdout/model-semantic.diff lines 9-11, connect allowOwnerBypass to the existing restrictive authorization decision so an empty ownerID permits non-admin access; in examples/code_review_agent/testdata/holdout/model-transaction-semantic.diff lines 8-11, update the transaction flow to mutate shared or persisted status before commit so a failed commit leaves observable inconsistent state.examples/code_review_agent/skills/code-review/scripts/check_rules.py-51-61 (1)
51-61: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPrivate-key redaction branch is unreachable given per-line invocation.
redact()is only ever called with a single diff line'sevidencetext, but this regex requires both the BEGIN and END markers in the same string — a real PEM block spans multiple separate "+" lines. Meanwhilesecret_value_pattern(line 68) matches on the BEGIN marker alone, soshould_report_secretdoes raise a finding on that header line, yetredact()then fails to mask it since its own pattern never matches a single-line header.🛠️ Proposed fix: also redact the header alone
text = re.sub(r"-----BEGIN [A-Z ]*PRIVATE KEY-----.*?-----END [A-Z ]*PRIVATE KEY-----", "[REDACTED_PRIVATE_KEY]", text) + text = re.sub(r"-----BEGIN [A-Z ]*PRIVATE KEY-----", "[REDACTED_PRIVATE_KEY]", text)🤖 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/check_rules.py` around lines 51 - 61, Update redact() so the private-key pattern also matches and replaces a standalone PEM BEGIN header, not only a complete BEGIN/END block. Preserve the existing full-block redaction and ensure the single-line header is masked consistently with secret_value_pattern and per-line evidence processing.examples/code_review_agent/internal/rules/rules.go-438-461 (1)
438-461: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid matching on any earlier loop in the hunk.
reportsDeferInLoopandreportsStringConcatLoopboth usecontainsAny(hunkBefore, "for ", "range "), so a laterdeferor+=can still be attributed to an unrelated, already-closed loop earlier in the same hunk. Tracking the active loop/braces instead would reduce these false positives.🤖 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/rules/rules.go` around lines 438 - 461, The loop detection in reportsDeferInLoop and reportsStringConcatLoop must only associate statements with an active enclosing loop, not any earlier “for ” or “range ” in hunkBefore. Replace the broad hunk-level containsAny checks with brace-aware tracking that determines whether the target defer or += occurs inside the current loop, preserving existing string-concatenation and defer matching behavior.examples/code_review_agent/internal/llm/http.go-116-119 (1)
116-119: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRedact decode errors for consistency with the status-code error path.
Line 114 redacts the response body via
review.RedactSecretsbefore wrapping into an error, but the JSON-decode failure path here does not, unlikeDecodeOutputinbridge.gowhich explicitly redacts decode errors. For defense-in-depth against secrets potentially surfacing in decode error text, redact here too.🛡️ Proposed fix
var output Output if err := json.Unmarshal(responseBody, &output); err != nil { - return Output{}, fmt.Errorf("decode model response: %w", err) + return Output{}, fmt.Errorf("decode model response: %s", review.RedactSecrets(err.Error())) }🤖 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/llm/http.go` around lines 116 - 119, Update the JSON unmarshalling error path in the response-decoding flow to redact the decode error before wrapping and returning it, matching the existing redaction used for status-code errors and by DecodeOutput in bridge.go. Preserve the current error context and wrapped-error behavior while ensuring raw decode details cannot expose secrets.examples/code_review_agent/internal/agent/agent_test.go-1026-1050 (1)
1026-1050: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that
cr_agent.exception_countis actually emitted.The key is absent from the required-attribute list, so the later zero-value map lookup passes when the expected count is zero even if telemetry omitted it.
Proposed fix
"cr_agent.exception_counts", + "cr_agent.exception_count", "cr_agent.conclusion_status",Also applies to: 1109-1111
🤖 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/agent/agent_test.go` around lines 1026 - 1050, Add cr_agent.exception_count to the required attribute list in the test’s telemetry key assertions, alongside the other agent count metrics. Ensure the later assertion validates the emitted attribute rather than allowing a missing key to appear as a zero value.examples/code_review_agent/internal/execution/exec.go-58-65 (1)
58-65: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up the local fallback workdir
NewExecutorallocates a temp dir forRuntimeLocalFallback, butAgent.Closeonly closes storage. Remove that directory on shutdown, or return a cleanup hook fromNewExecutor, so local runs don’t leave temp dirs behind.🤖 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/execution/exec.go` around lines 58 - 65, Update NewExecutor’s RuntimeLocalFallback setup and Agent.Close to clean up the directory created by os.MkdirTemp. Propagate a cleanup hook or retain the workdir path so Agent.Close removes it during shutdown, while preserving executor initialization and error handling.examples/code_review_agent/internal/agent/skill_step.go-73-78 (1)
73-78: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRound the timeout up before marshalling it.
int(a.cfg.Timeout.Seconds())drops sub-second values to0, andskill_runtreats0as the default timeout, so a 500ms config would run for 5m instead of the configured value and diverge fromTimeoutMS.Suggested fix
- "timeout": int(a.cfg.Timeout.Seconds()), + "timeout": int(math.Ceil(a.cfg.Timeout.Seconds())),🤖 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/agent/skill_step.go` around lines 73 - 78, Round the configured timeout up before converting it for the runArgs payload, replacing the truncating conversion in the skill command setup. Ensure sub-second values such as 500ms serialize to at least 1 second and remain consistent with the existing TimeoutMS behavior.
🧹 Nitpick comments (6)
examples/code_review_agent/internal/review/types_test.go (1)
28-40: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRealistic secret-shaped literals duplicated across three test files trip secret scanners. Betterleaks already flags GitHub PAT/private-key shapes in
types_test.go; the same dummy shapes (sk-,sk-proj-,llm-live-,github_pat_,ghp_, JWT-like, PEM key) are repeated verbatim inskill_test.goandfixtures_test.go. Automated secret scanners (GitHub push protection, gitleaks, etc.) can't tell these are fixtures, risking blocked pushes or noisy security alerts on every touch of these files.
examples/code_review_agent/internal/review/types_test.go#L28-L40: introduce one shared helper/constant set of "obviously fake" secret shapes (e.g. insert a recognizable marker segment, or add scanner allowlist/suppression comments) and reuse it here.examples/code_review_agent/internal/review/skill_test.go#L112-L120: reference the same shared fixture constants instead of re-declaring the raw strings.examples/code_review_agent/cmd/review-agent/fixtures_test.go#L59-L66: reference the same shared fixture constants instead of re-declaring the raw strings.🤖 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/types_test.go` around lines 28 - 40, The test fixtures use realistic secret-shaped literals that trigger automated scanners. In examples/code_review_agent/internal/review/types_test.go:28-40, define shared obviously-fake secret fixture constants or helpers with recognizable non-secret markers; in examples/code_review_agent/internal/review/skill_test.go:112-120 and examples/code_review_agent/cmd/review-agent/fixtures_test.go:59-66, replace the duplicated raw literals with references to those shared fixtures, preserving the existing test coverage.Source: Linters/SAST tools
examples/code_review_agent/internal/review/types.go (1)
147-155: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSHA-1 flagged by static analysis; consider SHA-256 for dedupe key.
Not a security-sensitive use (just a stable dedup fingerprint), but the ast-grep hint (CWE-327) will keep firing every scan since
**/*.gois linted viagolangci-lint/SAST in CI. Swapping tosha256.Sum256removes the recurring noise at near-zero cost.♻️ Proposed fix
- "crypto/sha1" + "crypto/sha256" ... - sum := sha1.Sum([]byte(strings.Join([]string{ + sum := sha256.Sum256([]byte(strings.Join([]string{🤖 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/types.go` around lines 147 - 155, Update Finding.DedupeKey to use SHA-256 via sha256.Sum256 instead of SHA-1, preserving the existing normalized field order, delimiter, and hex-encoded output format.Source: Linters/SAST tools
examples/code_review_agent/skills/code-review/scripts/check_fallback.go (1)
37-42: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo bounds check on
os.Args; fatal error path panics.
os.Args[1]will panic with an index-out-of-range if invoked without an argument, and aReadFilefailure panics rather than exiting cleanly — somewhat ironic given this same tool flagspanic(err)as thepanic-directanti-pattern in reviewed code.♻️ Proposed fix
func main() { - path := os.Args[1] + if len(os.Args) < 2 { + fmt.Fprintln(os.Stderr, "usage: check_fallback <diff-file>") + os.Exit(1) + } + path := os.Args[1] data, err := os.ReadFile(path) if err != nil { - panic(err) + fmt.Fprintln(os.Stderr, err) + os.Exit(1) }🤖 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/check_fallback.go` around lines 37 - 42, Update main to validate that os.Args contains the required path argument before accessing os.Args[1], and exit cleanly with a concise usage error when it is missing. Replace the panic(err) handling for os.ReadFile failures with explicit error reporting and a non-panicking nonzero exit, preserving the existing file-processing flow for valid input.examples/code_review_agent/testdata/holdout/README.md (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix grammar: use a hyphen for the compound adjective.
"private-key-shaped" acts as a compound adjective modifying "secret leak" and should be hyphenated.
📝 Proposed fix
-- `holdout-secret-private-key.diff`: private-key shaped secret leak. +- `holdout-secret-private-key.diff`: private-key-shaped secret leak.🤖 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/testdata/holdout/README.md` at line 7, Update the description for holdout-secret-private-key.diff in README.md to hyphenate the compound adjective as “private-key-shaped secret leak,” preserving the existing meaning and wording otherwise.Source: Linters/SAST tools
examples/code_review_agent/internal/report/report.go (1)
30-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffLarge duplication between English and Chinese report builders.
BuildMarkdown/BuildMarkdownChineseand theirwrite*/write*Chinesehelper pairs are structurally identical, differing only in literal strings. Consider a single renderer parameterized by a locale/labels struct to reduce this duplication and future drift risk between the two languages.🤖 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/report/report.go` around lines 30 - 146, Refactor BuildMarkdown and BuildMarkdownChinese into one shared renderer parameterized by a locale/labels structure containing the language-specific headings, labels, and formatting text. Update the corresponding write* and write*Chinese helper pairs to use shared implementations with those labels, while preserving the existing English and Chinese output and language-specific finding metadata. Keep the public builders as thin wrappers selecting the appropriate locale.examples/code_review_agent/internal/agent/reports.go (1)
226-234: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid
appendfor merging two caller-owned slices.
append(findings, warnings...)can write intofindings's backing array beyond its length if it has spare capacity, silently mutating memory owned by the caller. Iterate the two slices separately instead to avoid this well-known Go aliasing hazard.♻️ Proposed fix
func redactionCount(findings, warnings []review.Finding) int { total := 0 - for _, finding := range append(findings, warnings...) { - if strings.Contains(finding.Evidence, "[REDACTED]") { - total++ - } + for _, finding := range findings { + if strings.Contains(finding.Evidence, "[REDACTED]") { + total++ + } + } + for _, finding := range warnings { + if strings.Contains(finding.Evidence, "[REDACTED]") { + total++ + } } return total }🤖 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/agent/reports.go` around lines 226 - 234, Update redactionCount to avoid appending warnings onto the caller-owned findings slice, which may mutate its backing array. Iterate over findings and warnings separately while applying the existing Evidence redaction check and total count behavior.
🤖 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/go.mod`:
- Around line 11-13: Upgrade the vulnerable dependencies in go.mod: move
github.com/docker/docker from v28.4.0+incompatible to a release containing the
listed advisory fixes, update go.opentelemetry.io/otel/sdk to v1.40.0 or newer,
and update google.golang.org/grpc to v1.79.3 or newer. Preserve the existing
dependency declarations and update related module checksums as required.
In `@examples/code_review_agent/internal/agent/agent.go`:
- Around line 145-155: Update defaultPermissionPolicy to build its command
allowlist using the configured check-enable flags rather than hardcoding true in
both AllowedReviewCommands calls. Ensure disabled checks such as staticcheck are
excluded from the returned permission policy while enabled checks retain their
existing commands.
In `@examples/code_review_agent/internal/agent/output_parse.go`:
- Around line 88-105: Update sandboxCommandOutput and its callers to propagate
marshal/unmarshal errors instead of returning an empty commandOutput. Ensure
runGoSandboxCommand marks the check as failed whenever parsing executor output
fails, while preserving normal status, text, and exit-code handling for valid
output.
In `@examples/code_review_agent/internal/agent/runner_adapter.go`:
- Around line 48-55: Make event forwarding cancellation-aware in both sites:
examples/code_review_agent/internal/agent/runner_adapter.go:48-55 and
examples/code_review_agent/internal/agent/runner_adapter.go:68-88. In the
goroutines forwarding events, select between sending each event on out and
receiving from ctx.Done(), including both normal and error events; return on
cancellation so deferred Runner.Close execution is not blocked.
In `@examples/code_review_agent/internal/agent/sandbox_step.go`:
- Around line 56-159: Update runGoSandboxCommand so run.FinishedAt is assigned
before every return, including denied, fallback-denied, timed-out, error,
failed, and successful outcomes. Use a single completion timestamp representing
the function’s exit time while preserving all existing status and decision
behavior.
In `@examples/code_review_agent/internal/approval/gate.go`:
- Around line 71-100: Harden allowsCodeExecFallback by rejecting free-form shell
prefixes such as arbitrary cd commands and export GOCACHE segments; only approve
the exact configured fallback command or validate structured command fields.
Preserve the existing single code-block and allowed-command checks while
ensuring injected prefix commands cannot satisfy the fallback.
In `@examples/code_review_agent/internal/execution/exec.go`:
- Around line 103-117: Update ContainerHostConfig to set ReadonlyRootfs true and
add bounded tmpfs mounts for writable scratch paths such as /tmp and /go.
Preserve the existing resource limits and security settings while ensuring
temporary container storage cannot grow without bounds.
In `@examples/code_review_agent/internal/input/input.go`:
- Around line 159-174: Update resolveListedFile to canonicalize both baseDir and
the candidate path with filepath.EvalSymlinks before containment checks. Compute
the relative path between canonical locations and reject every candidate whose
relative path is ".." or starts with ".."+filepath.Separator, regardless of
restrictToBase or whether the original name was absolute; preserve the existing
display-name behavior for accepted files.
- Around line 85-93: Update the Git worktree path around isGitWorktree and
gitDiffArgs to include untracked files in the review, or explicitly report their
exclusion. Also update diffFromDirectory to recursively walk ordinary
directories and propagate read failures, ensuring neither fallback path silently
omits files; apply these changes at
examples/code_review_agent/internal/input/input.go lines 85-93 and 109-126.
In `@examples/code_review_agent/internal/llm/review.go`:
- Around line 207-226: Update NormalizeFinding so an omitted RuleID receives a
deterministic per-finding fallback derived from distinguishing fields such as
Title and evidence, rather than the shared "model-review" constant. Ensure the
generated identifier remains stable for equivalent findings and prevents
distinct findings at the same file, line, and category from colliding in
DedupeKey and MergeFindings.
In `@examples/code_review_agent/internal/report/report.go`:
- Around line 63-69: Make severity-count rendering deterministic in both
BuildMarkdown and BuildMarkdownChinese by collecting the keys from
result.Metrics.SeverityCounts, sorting them with sort.Strings, and iterating
over the sorted keys instead of ranging over the map directly. Add the sort
import and preserve the existing output formatting.
In `@examples/code_review_agent/internal/review/parser.go`:
- Around line 99-104: Update the diff-line parsing switch in the hunk parser so
lines beginning with “\” are handled by a dedicated no-op case and never reach
the default context-line handling. Ensure these metadata lines do not append a
Line, update currentHunk.Context, or increment oldLine/newLine, while preserving
existing behavior for actual context and change lines.
- Around line 77-89: Validate that the current file context exists before
accessing current.Path when handling hunk headers in the parser’s
HasPrefix(line, "@@ ") branch. For a hunk without a preceding file header,
return the existing invalid-diff error path instead of constructing Hunk with a
nil current context; preserve normal parsing for valid file headers.
In `@examples/code_review_agent/internal/review/types.go`:
- Around line 223-226: Update the private-key regex in the redaction rules to
use dotall mode via (?s), allowing the pattern to match PEM content across
newlines while preserving the existing BEGIN/END markers. Add a multiline PEM
fixture to the relevant RedactSecrets tests and verify it is replaced with
[REDACTED_PRIVATE_KEY].
In `@examples/code_review_agent/internal/storage/sqlite/sqlite.go`:
- Around line 276-326: Update SaveReview to enforce rec.Task.ID as the owner for
every child row, validating or normalizing each Decisions, FilterDecisions,
SandboxRuns, Findings, Metrics, and Artifacts record before persistence instead
of trusting item.TaskID. Make the save repeatable within the transaction by
deleting existing child rows for the task or using stable upserts, including
findings that currently conflict on their primary key, while preserving atomic
rollback behavior.
In `@examples/code_review_agent/skills/code-review/scripts/check_fallback.go`:
- Around line 170-189: The four lifecycle and resource guards in the surrounding
analysis logic incorrectly inspect localHunkText, which excludes following
cleanup lines. Replace those checks’ localHunkText references with hunkText,
matching the other checks, so deferred cancellation, closing, rollback, and
completion guards in the full hunk are recognized.
- Around line 85-114: The addFinding and addWarning closures currently
deduplicate most findings only by file and rule, dropping valid hits on
different lines. Use the same File|Line|Category|RuleID key produced by
Finding.DedupeKey() for every rule, and apply the identical change to the
string-concat-loop deduplication path.
In `@examples/code_review_agent/skills/code-review/scripts/check_rules.py`:
- Around line 322-326: Generalize the missing-test check in the rule containing
missing-test-hint to apply to any Go source file rather than only current_file
values "foo.go" and "service.go". Retain the exclusions for test files, the func
declaration check, and the existing warning behavior.
---
Minor comments:
In `@examples/code_review_agent/internal/agent/agent_test.go`:
- Around line 1026-1050: Add cr_agent.exception_count to the required attribute
list in the test’s telemetry key assertions, alongside the other agent count
metrics. Ensure the later assertion validates the emitted attribute rather than
allowing a missing key to appear as a zero value.
In `@examples/code_review_agent/internal/agent/skill_step.go`:
- Around line 73-78: Round the configured timeout up before converting it for
the runArgs payload, replacing the truncating conversion in the skill command
setup. Ensure sub-second values such as 500ms serialize to at least 1 second and
remain consistent with the existing TimeoutMS behavior.
In `@examples/code_review_agent/internal/execution/exec.go`:
- Around line 58-65: Update NewExecutor’s RuntimeLocalFallback setup and
Agent.Close to clean up the directory created by os.MkdirTemp. Propagate a
cleanup hook or retain the workdir path so Agent.Close removes it during
shutdown, while preserving executor initialization and error handling.
In `@examples/code_review_agent/internal/llm/http.go`:
- Around line 116-119: Update the JSON unmarshalling error path in the
response-decoding flow to redact the decode error before wrapping and returning
it, matching the existing redaction used for status-code errors and by
DecodeOutput in bridge.go. Preserve the current error context and wrapped-error
behavior while ensuring raw decode details cannot expose secrets.
In `@examples/code_review_agent/internal/review/types.go`:
- Around line 267-270: Update MustJSON to handle the error returned by
json.MarshalIndent instead of discarding it; preserve successful output, but
panic with the marshal error when serialization fails so the helper’s Must
contract is enforced.
In `@examples/code_review_agent/internal/rules/rules.go`:
- Around line 438-461: The loop detection in reportsDeferInLoop and
reportsStringConcatLoop must only associate statements with an active enclosing
loop, not any earlier “for ” or “range ” in hunkBefore. Replace the broad
hunk-level containsAny checks with brace-aware tracking that determines whether
the target defer or += occurs inside the current loop, preserving existing
string-concatenation and defer matching behavior.
In `@examples/code_review_agent/skills/code-review/scripts/check_rules.py`:
- Around line 51-61: Update redact() so the private-key pattern also matches and
replaces a standalone PEM BEGIN header, not only a complete BEGIN/END block.
Preserve the existing full-block redaction and ensure the single-line header is
masked consistently with secret_value_pattern and per-line evidence processing.
In `@examples/code_review_agent/testdata/holdout/model-semantic.diff`:
- Around line 9-11: Make the semantic holdouts reproduce their claimed
behaviors: in examples/code_review_agent/testdata/holdout/model-semantic.diff
lines 9-11, connect allowOwnerBypass to the existing restrictive authorization
decision so an empty ownerID permits non-admin access; in
examples/code_review_agent/testdata/holdout/model-transaction-semantic.diff
lines 8-11, update the transaction flow to mutate shared or persisted status
before commit so a failed commit leaves observable inconsistent state.
---
Nitpick comments:
In `@examples/code_review_agent/internal/agent/reports.go`:
- Around line 226-234: Update redactionCount to avoid appending warnings onto
the caller-owned findings slice, which may mutate its backing array. Iterate
over findings and warnings separately while applying the existing Evidence
redaction check and total count behavior.
In `@examples/code_review_agent/internal/report/report.go`:
- Around line 30-146: Refactor BuildMarkdown and BuildMarkdownChinese into one
shared renderer parameterized by a locale/labels structure containing the
language-specific headings, labels, and formatting text. Update the
corresponding write* and write*Chinese helper pairs to use shared
implementations with those labels, while preserving the existing English and
Chinese output and language-specific finding metadata. Keep the public builders
as thin wrappers selecting the appropriate locale.
In `@examples/code_review_agent/internal/review/types_test.go`:
- Around line 28-40: The test fixtures use realistic secret-shaped literals that
trigger automated scanners. In
examples/code_review_agent/internal/review/types_test.go:28-40, define shared
obviously-fake secret fixture constants or helpers with recognizable non-secret
markers; in examples/code_review_agent/internal/review/skill_test.go:112-120 and
examples/code_review_agent/cmd/review-agent/fixtures_test.go:59-66, replace the
duplicated raw literals with references to those shared fixtures, preserving the
existing test coverage.
In `@examples/code_review_agent/internal/review/types.go`:
- Around line 147-155: Update Finding.DedupeKey to use SHA-256 via sha256.Sum256
instead of SHA-1, preserving the existing normalized field order, delimiter, and
hex-encoded output format.
In `@examples/code_review_agent/skills/code-review/scripts/check_fallback.go`:
- Around line 37-42: Update main to validate that os.Args contains the required
path argument before accessing os.Args[1], and exit cleanly with a concise usage
error when it is missing. Replace the panic(err) handling for os.ReadFile
failures with explicit error reporting and a non-panicking nonzero exit,
preserving the existing file-processing flow for valid input.
In `@examples/code_review_agent/testdata/holdout/README.md`:
- Line 7: Update the description for holdout-secret-private-key.diff in
README.md to hyphenate the compound adjective as “private-key-shaped secret
leak,” preserving the existing meaning and wording otherwise.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba616eb4-2004-4722-97b8-72e85b82fb9a
⛔ Files ignored due to path filters (2)
examples/code_review_agent/go.sumis excluded by!**/*.sumexamples/code_review_agent/testdata/holdout/expected.tsvis excluded by!**/*.tsv
📒 Files selected for processing (100)
examples/code_review_agent/.gitignoreexamples/code_review_agent/DESIGN.mdexamples/code_review_agent/README.mdexamples/code_review_agent/cmd/review-agent/config.goexamples/code_review_agent/cmd/review-agent/config_test.goexamples/code_review_agent/cmd/review-agent/fixtures_test.goexamples/code_review_agent/cmd/review-agent/main.goexamples/code_review_agent/cmd/review-agent/main_test.goexamples/code_review_agent/cmd/review-agent/repo_test.goexamples/code_review_agent/cmd/review-agent/run.goexamples/code_review_agent/cr-agent.example.yamlexamples/code_review_agent/go.modexamples/code_review_agent/internal/agent/agent.goexamples/code_review_agent/internal/agent/agent_fast_test.goexamples/code_review_agent/internal/agent/agent_test.goexamples/code_review_agent/internal/agent/events.goexamples/code_review_agent/internal/agent/execution_plan.goexamples/code_review_agent/internal/agent/input_adapter.goexamples/code_review_agent/internal/agent/llm_step.goexamples/code_review_agent/internal/agent/output_parse.goexamples/code_review_agent/internal/agent/persist.goexamples/code_review_agent/internal/agent/reports.goexamples/code_review_agent/internal/agent/runner_adapter.goexamples/code_review_agent/internal/agent/sandbox_step.goexamples/code_review_agent/internal/agent/skill_step.goexamples/code_review_agent/internal/approval/gate.goexamples/code_review_agent/internal/approval/gate_test.goexamples/code_review_agent/internal/execution/exec.goexamples/code_review_agent/internal/execution/exec_test.goexamples/code_review_agent/internal/input/input.goexamples/code_review_agent/internal/input/input_test.goexamples/code_review_agent/internal/llm/bridge.goexamples/code_review_agent/internal/llm/bridge_test.goexamples/code_review_agent/internal/llm/http.goexamples/code_review_agent/internal/llm/openai.goexamples/code_review_agent/internal/llm/openai_test.goexamples/code_review_agent/internal/llm/review.goexamples/code_review_agent/internal/report/report.goexamples/code_review_agent/internal/report/report_test.goexamples/code_review_agent/internal/review/parser.goexamples/code_review_agent/internal/review/parser_test.goexamples/code_review_agent/internal/review/skill.goexamples/code_review_agent/internal/review/skill_test.goexamples/code_review_agent/internal/review/types.goexamples/code_review_agent/internal/review/types_test.goexamples/code_review_agent/internal/rules/rules.goexamples/code_review_agent/internal/rules/rules_test.goexamples/code_review_agent/internal/storage/sqlite/sqlite.goexamples/code_review_agent/internal/storage/sqlite/sqlite_test.goexamples/code_review_agent/internal/storage/store.goexamples/code_review_agent/review_report.jsonexamples/code_review_agent/review_report.mdexamples/code_review_agent/review_report.zh.mdexamples/code_review_agent/skills/code-review/SKILL.mdexamples/code_review_agent/skills/code-review/rules.mdexamples/code_review_agent/skills/code-review/scripts/check.shexamples/code_review_agent/skills/code-review/scripts/check_fallback.goexamples/code_review_agent/skills/code-review/scripts/check_rules.pyexamples/code_review_agent/testdata/fixtures/README.mdexamples/code_review_agent/testdata/fixtures/bare-return-err.diffexamples/code_review_agent/testdata/fixtures/command-injection.diffexamples/code_review_agent/testdata/fixtures/context-background.diffexamples/code_review_agent/testdata/fixtures/context.diffexamples/code_review_agent/testdata/fixtures/db-lifecycle.diffexamples/code_review_agent/testdata/fixtures/dedupe.diffexamples/code_review_agent/testdata/fixtures/defer-in-loop.diffexamples/code_review_agent/testdata/fixtures/goroutine.diffexamples/code_review_agent/testdata/fixtures/http-body.diffexamples/code_review_agent/testdata/fixtures/missing-test.diffexamples/code_review_agent/testdata/fixtures/mutex-unlock.diffexamples/code_review_agent/testdata/fixtures/panic.diffexamples/code_review_agent/testdata/fixtures/realistic-service-risk.diffexamples/code_review_agent/testdata/fixtures/resource.diffexamples/code_review_agent/testdata/fixtures/safe.diffexamples/code_review_agent/testdata/fixtures/sandbox-fail.diffexamples/code_review_agent/testdata/fixtures/sandbox-timeout.diffexamples/code_review_agent/testdata/fixtures/secret-shapes.diffexamples/code_review_agent/testdata/fixtures/secret.diffexamples/code_review_agent/testdata/fixtures/sql-string-concat.diffexamples/code_review_agent/testdata/fixtures/string-concat-loop.diffexamples/code_review_agent/testdata/fixtures/test-missing.diffexamples/code_review_agent/testdata/fixtures/todo.diffexamples/code_review_agent/testdata/holdout/README.mdexamples/code_review_agent/testdata/holdout/holdout-batch-worker-combo.diffexamples/code_review_agent/testdata/holdout/holdout-env-secret-guard.diffexamples/code_review_agent/testdata/holdout/holdout-expanded-go-risks.diffexamples/code_review_agent/testdata/holdout/holdout-expanded-safe-patterns.diffexamples/code_review_agent/testdata/holdout/holdout-guarded-lifecycle.diffexamples/code_review_agent/testdata/holdout/holdout-lifecycle-combo.diffexamples/code_review_agent/testdata/holdout/holdout-placeholder-secret.diffexamples/code_review_agent/testdata/holdout/holdout-pr-shaped-service.diffexamples/code_review_agent/testdata/holdout/holdout-safe-refactor.diffexamples/code_review_agent/testdata/holdout/holdout-secret-private-key.diffexamples/code_review_agent/testdata/holdout/model-authz-bypass.diffexamples/code_review_agent/testdata/holdout/model-error-swallow.diffexamples/code_review_agent/testdata/holdout/model-nil-boundary.diffexamples/code_review_agent/testdata/holdout/model-safe-semantic.diffexamples/code_review_agent/testdata/holdout/model-semantic.diffexamples/code_review_agent/testdata/holdout/model-state-inconsistency.diffexamples/code_review_agent/testdata/holdout/model-transaction-semantic.diff
| github.com/docker/docker v28.4.0+incompatible | ||
| go.opentelemetry.io/otel v1.38.0 | ||
| go.opentelemetry.io/otel/sdk v1.38.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
What is the latest version of google.golang.org/grpc and is it compatible with docker/docker v28.4.0?
💡 Result:
As of July 16, 2026, the latest version of google.golang.org/grpc is v1.82.1 [1]. Regarding compatibility, docker/docker v28.4.0 (also referred to as Moby Engine v28.4.0) [2] is a specific release that bundles its own dependencies at the time of its creation (September 2025) [2][3]. While Go modules allow for dependency updates, docker/docker v28.4.0 is not natively built against or explicitly tested for compatibility with the much newer v1.82.1 release of gRPC. If you are developing against the Docker codebase, you should use the versions defined in the project's go.mod file to ensure compatibility and stability [4][5].
Citations:
- 1: https://pkg.go.dev/google.golang.org/grpc
- 2: https://github.com/moby/moby/releases/tag/v28.4.0
- 3: https://packages.fedoraproject.org/pkgs/moby-engine/docker-cli/fedora-43.html
- 4: moby/moby@v28.0.0...v28.4.0
- 5: docker/cli@v28.3.3...v28.4.0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the module file with line numbers.
wc -l examples/code_review_agent/go.mod
cat -n examples/code_review_agent/go.mod | sed -n '1,140p'
# Look for the exact dependency versions and whether grpc is direct/indirect.
rg -n 'github.com/docker/docker|go.opentelemetry.io/otel(/sdk)?|google.golang.org/grpc' examples/code_review_agent/go.modRepository: trpc-group/trpc-agent-go
Length of output: 4853
🌐 Web query:
go.opentelemetry.io/otel/sdk GHSA-9h8m-3fm2-qjrq fixed version
💡 Result:
The vulnerability GHSA-9h8m-3fm2-qjrq (also identified as CVE-2026-24051) affects the OpenTelemetry Go SDK in versions v1.20.0 through v1.39.0 [1][2][3]. This vulnerability involves Path Hijacking (untrusted search paths) on macOS/Darwin systems, which could lead to arbitrary code execution [1][2]. The vulnerability has been patched in version v1.40.0 [4][5][6]. Users are advised to update to v1.40.0 or later to resolve this issue [4][2].
Citations:
- 1: https://github.com/github/advisory-database/blob/main/advisories/github-reviewed/2026/02/GHSA-9h8m-3fm2-qjrq/GHSA-9h8m-3fm2-qjrq.json
- 2: GHSA-9h8m-3fm2-qjrq
- 3: https://nvd.nist.gov/vuln/detail/cve-2026-24051
- 4: https://osv.dev/vulnerability/GHSA-9h8m-3fm2-qjrq
- 5: https://mondoo.com/vulnerability-intelligence/vulnerability/GHSA-9h8m-3fm2-qjrq
- 6: https://o3.security/vulnerability/GHSA-9h8m-3fm2-qjrq
🌐 Web query:
github.com/docker/docker v28.4.0 advisories fixed version OSV docker cp race authz plugin bypass PUT containers archive
💡 Result:
The Docker Engine and Moby project have faced several significant security vulnerabilities related to the docker cp command, the PUT /containers/{id}/archive API, and AuthZ plugin bypasses. These issues are detailed in the GitHub Advisory Database and OSV. Regarding the specific concerns mentioned: 1. PUT /containers/{id}/archive and docker cp race conditions (CVE-2026-41567, CVE-2026-41568): - CVE-2026-41567: This vulnerability involves incorrect ordering of operations when decompressing archives during PUT /containers/{id}/archive or docker cp requests. The daemon resolves external decompression binaries from the container's filesystem rather than the host's. A malicious container image containing a trojanized decompression binary can achieve code execution with daemon (host root) privileges [1][2]. - CVE-2026-41568: This is a race condition during docker cp mount setup. A malicious container can swap path components with symlinks, allowing the creation of arbitrary empty files or directories on the host filesystem with root privileges [3][4][5]. This was patched by scoping mountpoint creation to the container root using os.Root (Go 1.24+), preventing symlink escapes [3][4]. 2. AuthZ Plugin Bypass: - The issue involving AuthZ plugin bypass due to oversized request bodies is tracked as CVE-2026-34040 (GHSA-x744-4wpc-v9h2) [6][7]. It is noted as an incomplete fix for a previous vulnerability (CVE-2024-41110) [6][8]. An attacker can craft API requests that cause the daemon to forward the request to an authorization plugin without the body, potentially causing the plugin to approve a request it would otherwise deny [6][7]. 3. Versioning Context (v28.4.0): - Docker Engine version 28.4.0 was released on September 3, 2025 [9]. - Vulnerabilities such as CVE-2026-41567 and CVE-2026-41568 affect versions up to 28.5.2 [1][3][5]. - The AuthZ bypass (CVE-2026-34040) affects versions prior to 29.3.1 [6]. Users are advised to keep their Docker Engine installations updated to the latest versions (typically 29.5.1 or later for these specific fixes, depending on the release track) and to follow security best practices, such as running containers only from trusted images and restricting API access [4][5].
Citations:
- 1: GHSA-x86f-5xw2-fm2r
- 2: https://osv.dev/vulnerability/GHSA-x86f-5xw2-fm2r
- 3: GHSA-vp62-88p7-qqf5
- 4: https://scout.docker.com/vulnerabilities/id/CVE-2026-41568
- 5: https://app.opencve.io/cve/CVE-2026-41568
- 6: GHSA-x744-4wpc-v9h2
- 7: https://osv.dev/vulnerability/GHSA-x744-4wpc-v9h2
- 8: https://osv.dev/vulnerability/GHSA-v23v-6jw2-98fq
- 9: https://github.com/moby/moby/releases/tag/v28.4.0
🌐 Web query:
google.golang.org/grpc GHSA-p77j-4mvh-x3m3 fixed version v1.79.3
💡 Result:
Yes, v1.79.3 is the correct fixed version for GHSA-p77j-4mvh-x3m3 (also tracked as CVE-2026-33186) [1][2][3]. This vulnerability is an authorization bypass in gRPC-Go caused by improper input validation of the HTTP/2:path pseudo-header [4][5]. Previously, the server was overly lenient, accepting requests where the:path omitted the mandatory leading slash (e.g., Service/Method instead of /Service/Method) [4][5]. Because authorization interceptors often rely on canonical paths starting with a forward slash, this discrepancy allowed malformed requests to bypass security policies that included specific "deny" rules [4][5]. The fix in v1.79.3 ensures that any request with a:path that does not start with a leading slash is rejected with a codes.Unimplemented error, preventing the request from reaching authorization interceptors or handlers [4][5]. Users are advised to upgrade to v1.79.3 or newer to remediate this issue [5].
Citations:
- 1: GHSA-p77j-4mvh-x3m3
- 2: https://pkg.go.dev/vuln/GO-2026-4762
- 3: https://go.googlesource.com/vulndb/+/747f75d97d85e97cbc1658321c4df59d452c020a/data/reports/GO-2026-4762.yaml
- 4: https://osv.dev/vulnerability/GHSA-p77j-4mvh-x3m3
- 5: https://cvepremium.circl.lu/vuln/ghsa-p77j-4mvh-x3m3
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check where gRPC is used in the repo and whether there are any explicit version constraints.
rg -n 'google\.golang\.org/grpc|grpc-go|grpc-gateway' .
# Inspect module metadata for the code_review_agent example and its local replace targets.
cat -n examples/code_review_agent/go.mod | sed -n '1,120p'Repository: trpc-group/trpc-agent-go
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant module files with line numbers.
for f in go.mod codeexecutor/container/go.mod examples/code_review_agent/go.mod; do
echo "===== $f ====="
cat -n "$f" | sed -n '1,140p'
doneRepository: trpc-group/trpc-agent-go
Length of output: 12202
Upgrade these vulnerable dependencies
github.com/docker/docker v28.4.0+incompatibleis on a vulnerable Docker Engine release; move to a release that includes the fixes for the listed advisories.go.opentelemetry.io/otel/sdk v1.38.0is affected by GHSA-9h8m-3fm2-qjrq; bump tov1.40.0+.google.golang.org/grpc v1.67.0is below thev1.79.3fix for GHSA-p77j-4mvh-x3m3; bump tov1.79.3+.
🤖 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` around lines 11 - 13, Upgrade the
vulnerable dependencies in go.mod: move github.com/docker/docker from
v28.4.0+incompatible to a release containing the listed advisory fixes, update
go.opentelemetry.io/otel/sdk to v1.40.0 or newer, and update
google.golang.org/grpc to v1.79.3 or newer. Preserve the existing dependency
declarations and update related module checksums as required.
| func sandboxCommandOutput(raw any) commandOutput { | ||
| b, err := json.Marshal(raw) | ||
| if err != nil { | ||
| return commandOutput{} | ||
| } | ||
| var out struct { | ||
| Status string `json:"status"` | ||
| Output string `json:"output"` | ||
| ExitCode *int `json:"exit_code"` | ||
| } | ||
| if err := json.Unmarshal(b, &out); err != nil { | ||
| return commandOutput{} | ||
| } | ||
| return commandOutput{ | ||
| Status: out.Status, | ||
| Text: out.Output, | ||
| ExitCode: out.ExitCode, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not treat malformed executor output as a successful check.
Marshal or unmarshal failures currently return an empty commandOutput; runGoSandboxCommand then records the check as ok. Return the parsing error and mark the run as failed instead.
Proposed fix
-func sandboxCommandOutput(raw any) commandOutput {
+func sandboxCommandOutput(raw any) (commandOutput, error) {
b, err := json.Marshal(raw)
if err != nil {
- return commandOutput{}
+ return commandOutput{}, fmt.Errorf("marshal sandbox output: %w", err)
}
// ...
if err := json.Unmarshal(b, &out); err != nil {
- return commandOutput{}
+ return commandOutput{}, fmt.Errorf("decode sandbox output: %w", err)
}
- return commandOutput{
+ return commandOutput{
Status: out.Status,
Text: out.Output,
ExitCode: out.ExitCode,
- }
+ }, 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/agent/output_parse.go` around lines 88 -
105, Update sandboxCommandOutput and its callers to propagate marshal/unmarshal
errors instead of returning an empty commandOutput. Ensure runGoSandboxCommand
marks the check as failed whenever parsing executor output fails, while
preserving normal status, text, and exit-code handling for valid output.
| func (a *Agent) runGoSandboxCommand(ctx context.Context, taskID string, repoPath string, command string) ([]storage.DecisionRecord, storage.SandboxRunRecord) { | ||
| execCommand := execution.BoundedSandboxCommand( | ||
| execution.SandboxExecCommand(a.cfg.Runtime, command), | ||
| a.cfg.OutputLimitBytes, | ||
| ) | ||
| workspaceArgs, _ := execution.WorkspaceArgs(execCommand, a.cfg.Timeout, execution.SandboxEnv(a.cfg.Runtime)) | ||
| legacyArgs, _ := json.Marshal(map[string]any{ | ||
| "code_blocks": []map[string]string{{ | ||
| "language": "bash", | ||
| "code": execution.SandboxCode(a.cfg.Runtime, repoPath, execCommand), | ||
| }}, | ||
| "execution_id": taskID + "-" + strings.ReplaceAll(command, " ", "-"), | ||
| }) | ||
| permReq := &tool.PermissionRequest{ | ||
| Tool: a.checkTool, | ||
| ToolName: "workspace_exec", | ||
| Declaration: a.checkTool.Declaration(), | ||
| Arguments: workspaceArgs, | ||
| } | ||
| perm, err := a.policy.CheckToolPermission(ctx, permReq) | ||
| if err != nil { | ||
| perm = tool.DenyPermission(err.Error()) | ||
| } | ||
| perm, err = tool.NormalizePermissionDecision(perm) | ||
| if err != nil { | ||
| perm = tool.DenyPermission(err.Error()) | ||
| } | ||
| decision := storage.DecisionRecord{ | ||
| TaskID: taskID, Command: command, | ||
| Action: string(perm.Action), Reason: perm.Reason, At: time.Now(), | ||
| } | ||
| decisions := []storage.DecisionRecord{decision} | ||
| run := storage.SandboxRunRecord{ | ||
| TaskID: taskID, Command: command, Runtime: a.cfg.Runtime, | ||
| Status: "skipped", TimeoutMS: a.cfg.Timeout.Milliseconds(), | ||
| OutputLimitBytes: a.cfg.OutputLimitBytes, | ||
| EnvWhitelist: sandboxEnvWhitelist, | ||
| At: time.Now(), | ||
| } | ||
| if perm.Action != tool.PermissionActionAllow { | ||
| run.Status = string(perm.Action) | ||
| return decisions, run | ||
| } | ||
|
|
||
| start := time.Now() | ||
| run.ExecutionStarted = true | ||
| execCtx, cancel := context.WithTimeout(ctx, a.cfg.Timeout) | ||
| defer cancel() | ||
| raw, err := execution.RunWorkspaceCommand(execCtx, a.exec, repoPath, execCommand, a.cfg.Timeout, execution.SandboxEnv(a.cfg.Runtime)) | ||
| if err != nil { | ||
| fallbackReq := &tool.PermissionRequest{ | ||
| Tool: a.checkTool, | ||
| ToolName: "execute_code", | ||
| Declaration: a.checkTool.Declaration(), | ||
| Arguments: legacyArgs, | ||
| } | ||
| fallbackPerm, permissionErr := a.policy.CheckToolPermission(execCtx, fallbackReq) | ||
| if permissionErr != nil { | ||
| fallbackPerm = tool.DenyPermission(permissionErr.Error()) | ||
| } | ||
| fallbackPerm, permissionErr = tool.NormalizePermissionDecision(fallbackPerm) | ||
| if permissionErr != nil { | ||
| fallbackPerm = tool.DenyPermission(permissionErr.Error()) | ||
| } | ||
| decisions = append(decisions, storage.DecisionRecord{ | ||
| TaskID: taskID, Command: command, | ||
| Action: string(fallbackPerm.Action), Reason: fallbackPerm.Reason, At: time.Now(), | ||
| }) | ||
| if fallbackPerm.Action != tool.PermissionActionAllow { | ||
| run.Status = string(fallbackPerm.Action) | ||
| run.DurationMS = time.Since(start).Milliseconds() | ||
| return decisions, run | ||
| } | ||
| raw, err = a.checkTool.Call(execCtx, legacyArgs) | ||
| } | ||
| run.DurationMS = time.Since(start).Milliseconds() | ||
| if err != nil { | ||
| if execCtx.Err() != nil { | ||
| run.Status = "timed_out" | ||
| } else { | ||
| run.Status = "error" | ||
| } | ||
| run.StderrDigest = digestString(err.Error()) | ||
| return decisions, run | ||
| } | ||
| output := sandboxCommandOutput(raw) | ||
| run.StdoutDigest = digestString(output.Text) | ||
| run.Output = sandboxRunOutput(output.Text, a.cfg.OutputLimitBytes) | ||
| if output.ExitCode != nil { | ||
| run.ExitCode = *output.ExitCode | ||
| } | ||
| if output.Status != "" && output.Status != "exited" && output.ExitCode == nil { | ||
| run.Status = output.Status | ||
| return decisions, run | ||
| } | ||
| if run.ExitCode != 0 || strings.Contains(output.Text, "Error executing code block") { | ||
| run.Status = "failed" | ||
| if run.ExitCode == 0 { | ||
| run.ExitCode = 1 | ||
| } | ||
| return decisions, run | ||
| } | ||
| run.Status = "ok" | ||
| return decisions, run |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Record FinishedAt on every sandbox outcome.
This function never sets run.FinishedAt, leaving persisted audit records incomplete for allowed, denied, failed, and timed-out commands.
Proposed fix
-func (a *Agent) runGoSandboxCommand(...) ([]storage.DecisionRecord, storage.SandboxRunRecord) {
+func (a *Agent) runGoSandboxCommand(...) (
+ decisions []storage.DecisionRecord,
+ run storage.SandboxRunRecord,
+) {
// ...
- decisions := []storage.DecisionRecord{decision}
- run := storage.SandboxRunRecord{
+ decisions = []storage.DecisionRecord{decision}
+ run = storage.SandboxRunRecord{
// ...
}
+ defer func() {
+ run.FinishedAt = time.Now()
+ }()📝 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 (a *Agent) runGoSandboxCommand(ctx context.Context, taskID string, repoPath string, command string) ([]storage.DecisionRecord, storage.SandboxRunRecord) { | |
| execCommand := execution.BoundedSandboxCommand( | |
| execution.SandboxExecCommand(a.cfg.Runtime, command), | |
| a.cfg.OutputLimitBytes, | |
| ) | |
| workspaceArgs, _ := execution.WorkspaceArgs(execCommand, a.cfg.Timeout, execution.SandboxEnv(a.cfg.Runtime)) | |
| legacyArgs, _ := json.Marshal(map[string]any{ | |
| "code_blocks": []map[string]string{{ | |
| "language": "bash", | |
| "code": execution.SandboxCode(a.cfg.Runtime, repoPath, execCommand), | |
| }}, | |
| "execution_id": taskID + "-" + strings.ReplaceAll(command, " ", "-"), | |
| }) | |
| permReq := &tool.PermissionRequest{ | |
| Tool: a.checkTool, | |
| ToolName: "workspace_exec", | |
| Declaration: a.checkTool.Declaration(), | |
| Arguments: workspaceArgs, | |
| } | |
| perm, err := a.policy.CheckToolPermission(ctx, permReq) | |
| if err != nil { | |
| perm = tool.DenyPermission(err.Error()) | |
| } | |
| perm, err = tool.NormalizePermissionDecision(perm) | |
| if err != nil { | |
| perm = tool.DenyPermission(err.Error()) | |
| } | |
| decision := storage.DecisionRecord{ | |
| TaskID: taskID, Command: command, | |
| Action: string(perm.Action), Reason: perm.Reason, At: time.Now(), | |
| } | |
| decisions := []storage.DecisionRecord{decision} | |
| run := storage.SandboxRunRecord{ | |
| TaskID: taskID, Command: command, Runtime: a.cfg.Runtime, | |
| Status: "skipped", TimeoutMS: a.cfg.Timeout.Milliseconds(), | |
| OutputLimitBytes: a.cfg.OutputLimitBytes, | |
| EnvWhitelist: sandboxEnvWhitelist, | |
| At: time.Now(), | |
| } | |
| if perm.Action != tool.PermissionActionAllow { | |
| run.Status = string(perm.Action) | |
| return decisions, run | |
| } | |
| start := time.Now() | |
| run.ExecutionStarted = true | |
| execCtx, cancel := context.WithTimeout(ctx, a.cfg.Timeout) | |
| defer cancel() | |
| raw, err := execution.RunWorkspaceCommand(execCtx, a.exec, repoPath, execCommand, a.cfg.Timeout, execution.SandboxEnv(a.cfg.Runtime)) | |
| if err != nil { | |
| fallbackReq := &tool.PermissionRequest{ | |
| Tool: a.checkTool, | |
| ToolName: "execute_code", | |
| Declaration: a.checkTool.Declaration(), | |
| Arguments: legacyArgs, | |
| } | |
| fallbackPerm, permissionErr := a.policy.CheckToolPermission(execCtx, fallbackReq) | |
| if permissionErr != nil { | |
| fallbackPerm = tool.DenyPermission(permissionErr.Error()) | |
| } | |
| fallbackPerm, permissionErr = tool.NormalizePermissionDecision(fallbackPerm) | |
| if permissionErr != nil { | |
| fallbackPerm = tool.DenyPermission(permissionErr.Error()) | |
| } | |
| decisions = append(decisions, storage.DecisionRecord{ | |
| TaskID: taskID, Command: command, | |
| Action: string(fallbackPerm.Action), Reason: fallbackPerm.Reason, At: time.Now(), | |
| }) | |
| if fallbackPerm.Action != tool.PermissionActionAllow { | |
| run.Status = string(fallbackPerm.Action) | |
| run.DurationMS = time.Since(start).Milliseconds() | |
| return decisions, run | |
| } | |
| raw, err = a.checkTool.Call(execCtx, legacyArgs) | |
| } | |
| run.DurationMS = time.Since(start).Milliseconds() | |
| if err != nil { | |
| if execCtx.Err() != nil { | |
| run.Status = "timed_out" | |
| } else { | |
| run.Status = "error" | |
| } | |
| run.StderrDigest = digestString(err.Error()) | |
| return decisions, run | |
| } | |
| output := sandboxCommandOutput(raw) | |
| run.StdoutDigest = digestString(output.Text) | |
| run.Output = sandboxRunOutput(output.Text, a.cfg.OutputLimitBytes) | |
| if output.ExitCode != nil { | |
| run.ExitCode = *output.ExitCode | |
| } | |
| if output.Status != "" && output.Status != "exited" && output.ExitCode == nil { | |
| run.Status = output.Status | |
| return decisions, run | |
| } | |
| if run.ExitCode != 0 || strings.Contains(output.Text, "Error executing code block") { | |
| run.Status = "failed" | |
| if run.ExitCode == 0 { | |
| run.ExitCode = 1 | |
| } | |
| return decisions, run | |
| } | |
| run.Status = "ok" | |
| return decisions, run | |
| func (a *Agent) runGoSandboxCommand(ctx context.Context, taskID string, repoPath string, command string) ( | |
| decisions []storage.DecisionRecord, | |
| run storage.SandboxRunRecord, | |
| ) { | |
| execCommand := execution.BoundedSandboxCommand( | |
| execution.SandboxExecCommand(a.cfg.Runtime, command), | |
| a.cfg.OutputLimitBytes, | |
| ) | |
| workspaceArgs, _ := execution.WorkspaceArgs(execCommand, a.cfg.Timeout, execution.SandboxEnv(a.cfg.Runtime)) | |
| legacyArgs, _ := json.Marshal(map[string]any{ | |
| "code_blocks": []map[string]string{{ | |
| "language": "bash", | |
| "code": execution.SandboxCode(a.cfg.Runtime, repoPath, execCommand), | |
| }}, | |
| "execution_id": taskID + "-" + strings.ReplaceAll(command, " ", "-"), | |
| }) | |
| permReq := &tool.PermissionRequest{ | |
| Tool: a.checkTool, | |
| ToolName: "workspace_exec", | |
| Declaration: a.checkTool.Declaration(), | |
| Arguments: workspaceArgs, | |
| } | |
| perm, err := a.policy.CheckToolPermission(ctx, permReq) | |
| if err != nil { | |
| perm = tool.DenyPermission(err.Error()) | |
| } | |
| perm, err = tool.NormalizePermissionDecision(perm) | |
| if err != nil { | |
| perm = tool.DenyPermission(err.Error()) | |
| } | |
| decision := storage.DecisionRecord{ | |
| TaskID: taskID, Command: command, | |
| Action: string(perm.Action), Reason: perm.Reason, At: time.Now(), | |
| } | |
| decisions = []storage.DecisionRecord{decision} | |
| run = storage.SandboxRunRecord{ | |
| TaskID: taskID, Command: command, Runtime: a.cfg.Runtime, | |
| Status: "skipped", TimeoutMS: a.cfg.Timeout.Milliseconds(), | |
| OutputLimitBytes: a.cfg.OutputLimitBytes, | |
| EnvWhitelist: sandboxEnvWhitelist, | |
| At: time.Now(), | |
| } | |
| defer func() { | |
| run.FinishedAt = time.Now() | |
| }() | |
| if perm.Action != tool.PermissionActionAllow { | |
| run.Status = string(perm.Action) | |
| return decisions, run | |
| } | |
| start := time.Now() | |
| run.ExecutionStarted = true | |
| execCtx, cancel := context.WithTimeout(ctx, a.cfg.Timeout) | |
| defer cancel() | |
| raw, err := execution.RunWorkspaceCommand(execCtx, a.exec, repoPath, execCommand, a.cfg.Timeout, execution.SandboxEnv(a.cfg.Runtime)) | |
| if err != nil { | |
| fallbackReq := &tool.PermissionRequest{ | |
| Tool: a.checkTool, | |
| ToolName: "execute_code", | |
| Declaration: a.checkTool.Declaration(), | |
| Arguments: legacyArgs, | |
| } | |
| fallbackPerm, permissionErr := a.policy.CheckToolPermission(execCtx, fallbackReq) | |
| if permissionErr != nil { | |
| fallbackPerm = tool.DenyPermission(permissionErr.Error()) | |
| } | |
| fallbackPerm, permissionErr = tool.NormalizePermissionDecision(fallbackPerm) | |
| if permissionErr != nil { | |
| fallbackPerm = tool.DenyPermission(permissionErr.Error()) | |
| } | |
| decisions = append(decisions, storage.DecisionRecord{ | |
| TaskID: taskID, Command: command, | |
| Action: string(fallbackPerm.Action), Reason: fallbackPerm.Reason, At: time.Now(), | |
| }) | |
| if fallbackPerm.Action != tool.PermissionActionAllow { | |
| run.Status = string(fallbackPerm.Action) | |
| run.DurationMS = time.Since(start).Milliseconds() | |
| return decisions, run | |
| } | |
| raw, err = a.checkTool.Call(execCtx, legacyArgs) | |
| } | |
| run.DurationMS = time.Since(start).Milliseconds() | |
| if err != nil { | |
| if execCtx.Err() != nil { | |
| run.Status = "timed_out" | |
| } else { | |
| run.Status = "error" | |
| } | |
| run.StderrDigest = digestString(err.Error()) | |
| return decisions, run | |
| } | |
| output := sandboxCommandOutput(raw) | |
| run.StdoutDigest = digestString(output.Text) | |
| run.Output = sandboxRunOutput(output.Text, a.cfg.OutputLimitBytes) | |
| if output.ExitCode != nil { | |
| run.ExitCode = *output.ExitCode | |
| } | |
| if output.Status != "" && output.Status != "exited" && output.ExitCode == nil { | |
| run.Status = output.Status | |
| return decisions, run | |
| } | |
| if run.ExitCode != 0 || strings.Contains(output.Text, "Error executing code block") { | |
| run.Status = "failed" | |
| if run.ExitCode == 0 { | |
| run.ExitCode = 1 | |
| } | |
| return decisions, run | |
| } | |
| run.Status = "ok" | |
| return decisions, 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/agent/sandbox_step.go` around lines 56 -
159, Update runGoSandboxCommand so run.FinishedAt is assigned before every
return, including denied, fallback-denied, timed-out, error, failed, and
successful outcomes. Use a single completion timestamp representing the
function’s exit time while preserving all existing status and decision behavior.
| for _, item := range rec.Decisions { | ||
| if _, err = tx.ExecContext(ctx, `INSERT INTO permission_decisions(task_id, command, action, reason, created_at) VALUES(?, ?, ?, ?, ?)`, | ||
| item.TaskID, item.Command, item.Action, item.Reason, item.At.UTC().Format(time.RFC3339Nano)); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for _, item := range rec.FilterDecisions { | ||
| if _, err = tx.ExecContext(ctx, `INSERT INTO filter_decisions(task_id, target, action, reason, created_at) VALUES(?, ?, ?, ?, ?)`, | ||
| item.TaskID, item.Target, item.Action, item.Reason, item.At.UTC().Format(time.RFC3339Nano)); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for _, item := range rec.SandboxRuns { | ||
| if _, err = tx.ExecContext(ctx, ` | ||
| INSERT INTO sandbox_runs(task_id, command, runtime, status, timeout_ms, output_limit_bytes, env_whitelist, exit_code, stdout_digest, stderr_digest, duration_ms, output, created_at, finished_at, artifact_count) | ||
| VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, item.TaskID, item.Command, item.Runtime, item.Status, | ||
| item.TimeoutMS, item.OutputLimitBytes, item.EnvWhitelist, item.ExitCode, item.StdoutDigest, item.StderrDigest, | ||
| item.DurationMS, item.Output, item.At.UTC().Format(time.RFC3339Nano), nullableTime(item.FinishedAt), item.ArtifactCount); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for _, item := range rec.Findings { | ||
| key := item.DedupeKey() | ||
| if _, err = tx.ExecContext(ctx, ` | ||
| INSERT INTO findings(finding_id, task_id, severity, category, file, line, title, evidence, recommendation, confidence, source, rule_id, dedupe_key, status) | ||
| VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, rec.Task.ID+":"+key, rec.Task.ID, item.Severity, item.Category, | ||
| item.File, item.Line, item.Title, item.Evidence, item.Recommendation, item.Confidence, item.Source, item.RuleID, key, item.Status); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| m := rec.Metrics | ||
| if _, err = tx.ExecContext(ctx, ` | ||
| INSERT INTO metrics(task_id, mode, sandbox_requested, sandbox_executed, model_requested, model_executed, total_duration_ms, sandbox_duration_ms, model_duration_ms, tool_call_count, model_call_count, model_provider, model_name, model_backend, permission_block_count, finding_count, model_finding_count, model_exception_count, severity_counts_json, exception_counts_json, redaction_count, created_at) | ||
| VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
| ON CONFLICT(task_id) DO UPDATE SET total_duration_ms=excluded.total_duration_ms, sandbox_duration_ms=excluded.sandbox_duration_ms, | ||
| mode=excluded.mode, sandbox_requested=excluded.sandbox_requested, sandbox_executed=excluded.sandbox_executed, | ||
| model_requested=excluded.model_requested, model_executed=excluded.model_executed, | ||
| model_duration_ms=excluded.model_duration_ms, tool_call_count=excluded.tool_call_count, model_call_count=excluded.model_call_count, | ||
| model_provider=excluded.model_provider, model_name=excluded.model_name, model_backend=excluded.model_backend, | ||
| permission_block_count=excluded.permission_block_count, finding_count=excluded.finding_count, | ||
| model_finding_count=excluded.model_finding_count, model_exception_count=excluded.model_exception_count, | ||
| severity_counts_json=excluded.severity_counts_json, exception_counts_json=excluded.exception_counts_json, | ||
| redaction_count=excluded.redaction_count, created_at=excluded.created_at`, m.TaskID, m.Mode, m.SandboxRequested, m.SandboxExecuted, m.ModelRequested, m.ModelExecuted, m.TotalDurationMS, m.SandboxDurationMS, | ||
| m.ModelDurationMS, m.ToolCallCount, m.ModelCallCount, m.ModelProvider, m.ModelName, m.ModelBackend, | ||
| m.PermissionBlockCount, m.FindingCount, m.ModelFindingCount, m.ModelExceptionCount, m.SeverityCountsJSON, | ||
| m.ExceptionCountsJSON, m.RedactionCount, m.At.UTC().Format(time.RFC3339Nano)); err != nil { | ||
| return err | ||
| } | ||
| for _, item := range rec.Artifacts { | ||
| if _, err = tx.ExecContext(ctx, `INSERT INTO artifacts(task_id, name, kind, path, digest, size_bytes, created_at) VALUES(?, ?, ?, ?, ?, ?, ?)`, | ||
| item.TaskID, item.Name, item.Kind, item.Path, item.Digest, item.Size, item.At.UTC().Format(time.RFC3339Nano)); err != nil { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Enforce aggregate ownership and make SaveReview safely repeatable.
Child rows use mixed task IDs and are blindly inserted. A mismatched record can attach to another task, while retrying the same task duplicates audit rows or fails on the finding primary key. Validate/normalize every child to rec.Task.ID and replace existing child rows—or use stable upserts—inside this transaction.
Also applies to: 334-337
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 298-301: SQL query is built by concatenating a string literal with a variable and passed to a database/sql call (Query, Exec, QueryRow, Prepare, or their Context variants). String concatenation lets attacker-controlled input alter the query structure, enabling SQL injection. Use parameterized queries with placeholders ('?' or '') and pass the values as separate arguments instead of concatenating them into the query string.
Context: tx.ExecContext(ctx, INSERT INTO findings(finding_id, task_id, severity, category, file, line, title, evidence, recommendation, confidence, source, rule_id, dedupe_key, status) VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?), rec.Task.ID+":"+key, rec.Task.ID, item.Severity, item.Category,
item.File, item.Line, item.Title, item.Evidence, item.Recommendation, item.Confidence, item.Source, item.RuleID, key, item.Status)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-query-string-concat-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/code_review_agent/internal/storage/sqlite/sqlite.go` around lines
276 - 326, Update SaveReview to enforce rec.Task.ID as the owner for every child
row, validating or normalizing each Decisions, FilterDecisions, SandboxRuns,
Findings, Metrics, and Artifacts record before persistence instead of trusting
item.TaskID. Make the save repeatable within the transaction by deleting
existing child rows for the task or using stable upserts, including findings
that currently conflict on their primary key, while preserving atomic rollback
behavior.
The Skill's Python implementation and its Go fallback limited this warning to fixture-named files, leaving ordinary Go changes uncovered. Both paths now use the same non-test Go-file predicate and are covered by a dual-runtime regression test. Constraint: Skill output must remain equivalent when Python is unavailable Rejected: Upgrade example-local transitive dependencies | versions are owned by the shared container module Confidence: high Scope-risk: narrow Directive: Keep Python and Go fallback rule predicates behaviorally aligned Tested: go test ./internal/review -run TestSkillCheckScript -count=1; go test ./...; go vet ./... Not-tested: Upstream CI matrix
Rememorio
left a comment
Rememorio
left a comment
There was a problem hiding this comment.
I reviewed the changed lines and left 10 inline comments below. The comments focus on issues worth addressing before merge.
Reviewed the new code-review example across input collection, deterministic skill runtimes, container execution, model redaction, event streaming, and SQLite persistence. The main risks are multiline secret exposure to external models, incomplete repository inputs, runtime-dependent findings, leaked executors, and cancellation paths that do not terminate or persist a terminal task state. Targeted validation should cover multiline PEM redaction, symlink containment, staged/untracked Git inputs, forced Go-fallback parity, executor cleanup, container staticcheck, and cancellation without draining events.
中文
我看过这次变更的相关代码，在下面留下 10 条行内评论。评论聚焦在合并前值得处理的问题。
已检查新增代码评审示例的输入收集、确定性 Skill 运行时、容器执行、模型脱敏、事件流和 SQLite 持久化。主要风险包括：多行密钥可能泄露给外部模型、仓库输入不完整、不同运行时产生不同 findings、执行器资源泄漏，以及取消后未及时终止或未写入任务终态。建议重点补充多行 PEM 脱敏、符号链接越界、Git staged/untracked 输入、强制 Go fallback 一致性、执行器清理、容器 staticcheck 和取消后不消费事件等测试。
| with: `[REDACTED]`, | ||
| }, | ||
| { | ||
| re: regexp.MustCompile(`-----BEGIN [A-Z ]*PRIVATE KEY-----.*?-----END [A-Z ]*PRIVATE KEY-----`), |
There was a problem hiding this comment.
P0: Multiline private keys bypass outbound redaction
This regexp does not enable dot-all matching, so a normal multiline PEM block is left intact when RedactSecrets processes the full diff. With model review enabled, that raw block is then sent to the HTTP/OpenAI provider; the current holdout uses a single-line key and cannot catch this exposure.
Redact complete PEM blocks before the generic assignment replacement, using multiline/dot-all matching, and add a provider-input test with a multiline Go raw string that asserts neither the PEM markers nor payload leave the process.
中文
多行私钥会绕过模型出站脱敏
这里的正则没有开启跨行匹配，因此常见的多行 PEM 私钥在 RedactSecrets 处理完整 diff 时不会被替换。启用真实模型后，原始私钥会继续发送到 HTTP/OpenAI Provider；当前 holdout 只有单行私钥，覆盖不到这个泄漏路径。
建议在通用键值替换之前先用可跨行的规则整体移除 PEM 块，并增加一个捕获 Provider 入参的测试：输入包含 Go 多行 raw string，断言 PEM 头尾和中间载荷都不会离开进程。
| taskStarted := false | ||
| defer func() { | ||
| if err != nil && taskStarted && a.store != nil { | ||
| _ = a.saveTaskStatus(ctx, taskID, inputRef, digestBytes(diff), req.RepoPath, mode, "failed", start, time.Now()) |
There was a problem hiding this comment.
P1: Canceled reviews can remain running in SQLite
This failure cleanup reuses the request context and discards the write error. Once that context is canceled, SaveTask cannot update the already-created running row, leaving replay and audit consumers with a task that never reaches a terminal state.
Persist the terminal failed or canceled state using a short bounded context derived with context.WithoutCancel, and handle the cleanup error without masking the original failure. Add a test that cancels after the initial running record and then verifies the stored terminal status.
中文
取消后的任务可能一直停留在 running
失败清理复用了请求 context，并且直接忽略写入错误。一旦该 context 已取消，SaveTask 就无法更新之前创建的 running 记录，回放和审计侧会看到一个永远没有终态的任务。
建议使用 context.WithoutCancel 派生一个带短超时的清理 context，写入 failed 或 canceled 终态，同时在不覆盖原始错误的前提下处理持久化失败。增加测试：在 running 记录写入后取消请求，再断言数据库中的任务已进入终态。
There was a problem hiding this comment.
The cancellation-safe terminal write is in place, but its error is still discarded. Please join or otherwise surface that cleanup error without hiding the original failure.
中文
取消后的终态写入已经改为使用独立的有界 context，但写入错误仍被丢弃。请在不掩盖原始失败的前提下合并或记录该清理错误。
| for _, run := range runs { | ||
| a.emitReviewEvent(ctx, taskID, reviewEventSandboxRun, run.Command) | ||
| } | ||
| if runErr != nil { |
There was a problem hiding this comment.
P1: Caller cancellation is downgraded to a review warning
runErr can be caused by the caller canceling ctx, but every error is converted into a human-review item and the pipeline continues into model calls, report generation, artifacts, and persistence. With persistence disabled and the default in-memory artifact service, a canceled Run can even complete with a normal result instead of returning context.Canceled.
Check ctx.Err() before applying the recoverable-execution fallback and return cancellation/deadline errors immediately. Apply the same distinction to model-provider errors and sandbox loops, with a test that verifies errors.Is and that later stages are not invoked.
中文
调用方取消被降级为普通审查告警
runErr 可能来自调用方取消 ctx，但这里把所有错误都转换成人工复核项，随后继续执行模型调用、报告生成、artifact 和持久化。关闭持久化并使用默认内存 artifact 服务时，一个已经取消的 Run 甚至可能正常返回结果，而不是返回 context.Canceled。
建议在进入可降级错误处理前先检查 ctx.Err()，对取消和截止时间错误立即返回。模型 Provider 和沙箱循环也应采用同样区分，并增加测试验证 errors.Is 保持成立且后续阶段不会继续执行。
| } | ||
|
|
||
| // Close 释放 Agent 持有的存储连接。 | ||
| func (a *Agent) Close() error { |
There was a problem hiding this comment.
P1: Close does not release the code executor
The default executor starts a long-running container, but Agent.Close returns immediately when no store is configured and otherwise closes only SQLite. Normal defer ag.Close() therefore leaves the container running until a nondeterministic finalizer; local fallback work directories are also never removed. Constructor failures after executor creation have the same leak.
Make the executor/work-directory cleanup an owned closer on Agent, invoke it from Close, and unwind it on every New error path after creation. A fake closer test should assert cleanup occurs exactly once with and without SQLite.
中文
Close 未释放代码执行器
默认执行器会启动一个长期运行的容器，但未配置 Store 时 Agent.Close 会直接返回，配置了 Store 时也只关闭 SQLite。因此常规的 defer ag.Close() 仍会留下容器，只能等待不确定的 finalizer；local fallback 创建的临时工作目录同样不会删除。执行器创建成功后如果构造流程失败，也存在相同泄漏。
建议让 Agent 显式持有执行器和临时目录的清理函数，在 Close 以及 New 创建执行器后的所有失败分支中调用。可用 fake closer 验证无论是否启用 SQLite 都只清理一次。
| defer close(out) | ||
| defer r.Close() | ||
| for ev := range events { | ||
| out <- ev |
There was a problem hiding this comment.
P1: Abandoned event consumers leak the runner goroutine
This unbuffered send does not observe ctx.Done(). If a RunWithEvents caller cancels and stops draining the channel, the forwarding goroutine blocks here indefinitely, so its deferred r.Close() never runs and runner/session resources remain live.
Forward with a select on the output channel and ctx.Done(), returning on cancellation so cleanup executes. Add a test that starts the stream, does not drain it, cancels the context, and verifies the stream and runner terminate promptly.
中文
停止消费事件会泄漏 Runner goroutine
这个无缓冲发送没有监听 ctx.Done()。如果 RunWithEvents 调用方取消后停止消费事件，转发 goroutine 会永久阻塞在这里，延迟执行的 r.Close() 也不会运行，Runner 和 Session 资源会持续存活。
建议使用同时监听输出通道和 ctx.Done() 的 select，取消时直接返回以触发清理。增加一个不继续消费事件、随后取消 context 的测试，验证事件流和 Runner 能及时结束。
| func AllowedReviewCommands(enableStaticcheck bool) []string { | ||
| commands := []string{"go test ./...", "go vet ./..."} | ||
| if enableStaticcheck { | ||
| commands = append(commands, "staticcheck ./...") |
There was a problem hiding this comment.
P1: Container staticcheck is never provisioned
Enabling staticcheck always schedules this command, but the configured golang:1.25-bookworm image does not provide the staticcheck binary. The container also has networking disabled, so it cannot install the tool during execution. The advertised production-container path therefore records a failed sandbox run instead of performing static analysis.
Use a pinned image that includes a pinned staticcheck binary, stage a vetted binary explicitly, or reject this capability for runtimes that cannot provide it. The container integration test should assert the staticcheck run completes successfully.
中文
默认容器未提供 staticcheck
启用 staticcheck 后这里会无条件加入命令，但配置的 golang:1.25-bookworm 镜像并不包含 staticcheck 可执行文件；同时容器禁用了网络，运行时也无法安装。因此宣传的生产容器路径只会记录一次失败的沙箱运行，并未真正执行静态分析。
建议使用内置固定版本 staticcheck 的镜像、显式注入经过校验的二进制，或在运行时不具备该能力时拒绝配置。容器集成测试应断言 staticcheck 的运行状态确实成功。
There was a problem hiding this comment.
The fail-fast guard resolves the execution failure. Please also update DESIGN.md to state that staticcheck is unavailable in the default container, or provision that capability.
中文
提前拒绝已经解决了必然执行失败的问题。还请同步修改 DESIGN.md，明确默认容器不支持 staticcheck，或者为容器真正提供该能力。
| } | ||
|
|
||
| func gitDiffArgs(repoPath string, baseRef string, headRef string) []string { | ||
| args := []string{"-C", repoPath, "diff", "--no-ext-diff", "--no-textconv", "--unified=3"} |
There was a problem hiding this comment.
P1: Default repository review omits staged and untracked files
When no refs are supplied, this becomes plain git diff, which includes only unstaged changes to tracked files. The CLI infers the current repository when no explicit input is provided, so staged modifications and new untracked files can be silently omitted and an incomplete review can report no findings.
Define the workspace-input contract to include index and untracked changes, then collect all three sets safely. Add repository tests with one staged modification and one untracked Go file, asserting both appear in the generated diff.
中文
默认仓库审查会遗漏 staged 和 untracked 文件
未提供 refs 时，这里最终执行普通的 git diff，它只包含 tracked 文件的未暂存修改。CLI 在没有显式输入时会自动使用当前仓库，因此已暂存的修改和新建的 untracked 文件会被静默遗漏，最终可能基于不完整输入给出无问题结论。
建议明确工作区输入应同时包含工作树、索引和 untracked 文件，并安全地收集这三类变更。增加包含一个 staged 修改和一个 untracked Go 文件的仓库测试，断言两者都进入生成的 diff。
There was a problem hiding this comment.
Root worktrees now include all three change sets, but subdirectory runs still resolve untracked names against the repository root. Use git ls-files --full-name or resolve names relative to the requested subdirectory, and add an untracked-subdirectory test.
中文
仓库根目录场景已经覆盖三类变更，但从子目录运行时仍会把 untracked 路径错误地拼到仓库根目录。请使用 git ls-files --full-name，或按请求子目录解析路径，并增加子目录 untracked 文件测试。
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fileContent, err := os.ReadFile(path) |
There was a problem hiding this comment.
P1: Symlinks bypass the repository boundary
The preceding containment check is lexical only. A listed path that is inside the repository but is a symlink to a host file outside it passes filepath.Rel, and os.ReadFile follows the link. That content is synthesized into the diff and can subsequently be written to reports or sent to an enabled model provider.
Resolve the canonical base and target before reading, or reject symlinked entries, then verify the resolved target remains below the repository root. Add a file-list test whose in-repository symlink points outside the fixture root.
中文
符号链接可绕过仓库目录边界
前面的目录约束只检查了字符串路径。仓库内的文件名如果是指向仓库外主机文件的符号链接，仍能通过 filepath.Rel，随后 os.ReadFile 会跟随链接读取内容。这些内容会被合成到 diff，并可能写入报告或发送给已启用的模型 Provider。
建议读取前解析基准目录和目标文件的真实路径，或直接拒绝符号链接，再确认真实目标仍位于仓库根目录内。同时增加一个“仓库内链接指向外部文件”的 file-list 测试。
| localHunkText := hunkBefore + "\n" + text | ||
|
|
||
| addFinding := func(severity, category, title, recommendation, ruleID string) { | ||
| key := currentFile + "|" + ruleID |
There was a problem hiding this comment.
P1: Fallback deduplication drops distinct findings
This key collapses every occurrence of a rule within one file. Two independent panic calls or resource leaks therefore produce only one finding under the Go fallback, whereas the Python engine and Finding.DedupeKey retain different lines. Review results consequently depend on whether Python happens to be installed.
Include line and category in both finding and warning keys, and run the existing two-panic assertion with the forced fallback environment.
中文
Fallback 去重会丢失不同位置的问题
这个 key 会把同一文件内同一规则的所有命中合并掉。两个不同位置的 panic 或资源泄漏在 Go fallback 下只会保留一个，而 Python 引擎和 Finding.DedupeKey 都会保留不同行号。因此审查结果会取决于环境中是否安装了 Python。
建议在 finding 和 warning 的 key 中都加入行号及类别，并让现有的双 panic 断言同时覆盖强制 fallback 环境。
| "Bind the goroutine to a context, wait group, or explicit completion signal.", "goroutine-leak") | ||
| } | ||
| if containsAny(text, "context.WithCancel", "context.WithTimeout", "context.WithDeadline") && | ||
| !containsAny(localHunkText, "defer cancel()", "ctx.Done", "cancel()") { |
There was a problem hiding this comment.
P1: Go fallback cannot see following cleanup
localHunkText contains only earlier and current added lines, and this check recognizes only the literal cancel() name. Idiomatic code such as ctx, stop := context.WithCancel(...); defer stop() on the next line is therefore reported as a high-severity leak. The resource and database checks below have the same look-ahead problem, while the Python engine uses the complete hunk and assigned variable.
Use the complete hunkText plus the assigned cleanup variable, matching the Python implementation. Run the existing cleanup scenario under the forced Go-fallback environment as well so both engines must produce identical findings.
中文
Go fallback 看不到后续清理语句
localHunkText 只包含当前行及之前的新增行，而且这里仅识别固定名称 cancel()。因此常见的 ctx, stop := context.WithCancel(...); defer stop() 会因为清理语句位于下一行而被误报为高严重级别泄漏。下面的资源和数据库规则也有同样的前向不可见问题，而 Python 实现使用的是完整 hunk 和实际变量名。
建议改用完整的 hunkText，并根据赋值语句提取真实的 cleanup 变量，与 Python 实现保持一致。同时让现有 cleanup 场景也在强制 Go fallback 环境下运行，确保两个引擎输出一致。
PEM blocks commonly span several lines, but the outbound redactor only matched a single line. Redact complete private-key blocks before generic assignment patterns and preserve the specific redaction marker. Constraint: Model-provider inputs must never retain raw private-key markers or payloads Rejected: Rely on the generic key-value rule | it cannot safely consume multiline PEM data Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep complete-secret patterns ahead of generic token patterns Tested: go test -run TestRedactSecrets -count=1 ./internal/review Not-tested: End-to-end HTTP provider capture
A default repository review previously omitted staged and untracked changes, while a file list could follow an in-repository symlink to host content. Build the default input from all workspace change classes and resolve restricted file-list paths before reading. Constraint: Explicit base/head comparisons retain their existing Git semantics Rejected: Review only git diff output | it silently ignores index and new files Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep repository input collection independent from explicit ref comparisons Tested: go test ./internal/input Not-tested: Git submodules and unusual filesystem link behavior
The Go fallback collapsed distinct rule hits and ignored cleanup introduced later in a diff hunk. Give every finding a line-aware identity and inspect complete hunks with the assigned cleanup variable. Constraint: Python may be unavailable in supported review environments Rejected: Treat fallback output as lower fidelity | review results must not depend on the installed runtime Confidence: high Scope-risk: narrow Reversibility: clean Directive: Add every deterministic rule to Python and Go fallback parity coverage Tested: go test ./internal/review -run TestSkillCheckScriptFallbackParityForDuplicateRuleLines -count=1; go test ./internal/review -run TestSkillCheckScriptFallbackParityForFollowingCleanup -count=1 Not-tested: Arbitrary nested-control-flow cleanup patterns
Canceled requests could continue through reporting, leave persisted tasks running, and retain runner or executor resources. Preserve cancellation as a terminal result, persist its status with a bounded cleanup context, and give runners and executors deterministic shutdown. Constraint: The production container image has no staticcheck binary and runs without network access Rejected: Silently disable staticcheck for containers | callers must receive a truthful capability boundary Confidence: high Scope-risk: moderate Reversibility: clean Directive: New runtime capabilities require an executable integration test before they are advertised Tested: go test ./internal/agent ./internal/execution ./internal/approval Not-tested: Docker-backed shutdown and staticcheck installation path
The redaction unit test alone did not prove the outbound provider boundary. Capture a model-review input containing a multiline PEM block and assert that neither markers nor payload cross the process boundary. Constraint: Model providers may be external services Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep outbound-secret tests at the provider boundary Tested: go test ./internal/llm Not-tested: Live HTTP provider transport
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/agent/agent.go`:
- Around line 351-358: Update the review-check flow around runReviewChecks so
reviewEventSkillRun is emitted only when the code-review skill actually
executes, not for dry-run or unsupported E2B paths. Move emission to the real
skill execution stage or propagate an explicit execution flag, while preserving
the existing event ordering and sandbox-run event semantics.
In `@examples/code_review_agent/internal/input/input_test.go`:
- Around line 93-96: Update the symlink setup in the test around linkPath and
os.Symlink so expected environment-related creation failures call t.Skip instead
of failing the test; retain t.Fatalf for unexpected errors and preserve all
existing Read security assertions after successful symlink creation.
In `@examples/code_review_agent/internal/input/input.go`:
- Around line 155-163: Before os.ReadFile in the untracked-file loop, use
os.Lstat on the filepath.Join(repoRoot, filepath.FromSlash(name)) path and
reject entries that are not regular files, including symlinks, with an
appropriate error. Keep diffForNewFile unchanged and only read files validated
by the metadata check.
In `@examples/code_review_agent/internal/review/skill_test.go`:
- Around line 329-333: Update runSkillCheck so mustLookPath(t, "python3") runs
only in normal Python mode, not when env intentionally configures the Go
fallback. Preserve the existing bash command execution and ensure fallback tests
can run without Python installed.
🪄 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: d01f1ad1-33f3-4fe3-ac49-0fa1b44f2d27
📒 Files selected for processing (12)
examples/code_review_agent/internal/agent/agent.goexamples/code_review_agent/internal/agent/agent_test.goexamples/code_review_agent/internal/agent/runner_adapter.goexamples/code_review_agent/internal/execution/exec.goexamples/code_review_agent/internal/execution/exec_test.goexamples/code_review_agent/internal/input/input.goexamples/code_review_agent/internal/input/input_test.goexamples/code_review_agent/internal/llm/bridge_test.goexamples/code_review_agent/internal/review/skill_test.goexamples/code_review_agent/internal/review/types.goexamples/code_review_agent/internal/review/types_test.goexamples/code_review_agent/skills/code-review/scripts/check_fallback.go
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/code_review_agent/internal/agent/runner_adapter.go
- examples/code_review_agent/internal/review/types.go
- examples/code_review_agent/internal/execution/exec_test.go
- examples/code_review_agent/skills/code-review/scripts/check_fallback.go
- examples/code_review_agent/internal/execution/exec.go
- examples/code_review_agent/internal/agent/agent_test.go
| result, decisions, runs, toolCallCount, runErr := a.runReviewChecks(ctx, taskID, plan, req.RepoPath, diff) | ||
| if ctxErr := ctx.Err(); ctxErr != nil { | ||
| return review.Result{}, ctxErr | ||
| } | ||
| a.emitReviewEvent(ctx, taskID, reviewEventSkillRun, defaultSkillCommand) | ||
| for _, run := range runs { | ||
| a.emitReviewEvent(ctx, taskID, reviewEventSandboxRun, run.Command) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Emit reviewEventSkillRun only when the skill actually executes.
Dry-run and unsupported E2B paths return from runReviewChecks without running the skill, but this unconditional event tells sinks that code-review ran. Emit it from the actual skill stage or return an explicit execution flag.
中文
仅在 Skill 实际执行时发送 reviewEventSkillRun。
dry-run 和不支持的 E2B 路径不会执行 Skill，但当前逻辑仍向事件消费者报告 code-review 已运行。应从真实 Skill 执行点发送事件，或返回明确的执行标记。
As per path instructions, execution-path changes must preserve externally observable event ordering and 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/agent/agent.go` around lines 351 - 358,
Update the review-check flow around runReviewChecks so reviewEventSkillRun is
emitted only when the code-review skill actually executes, not for dry-run or
unsupported E2B paths. Move emission to the real skill execution stage or
propagate an explicit execution flag, while preserving the existing event
ordering and sandbox-run event semantics.
Source: Path instructions
Dry-run and unsupported runtimes reported a skill execution that never occurred, while untracked symlinks could still be followed into host content. Emit the event at the actual allowed invocation and accept only regular untracked files. Constraint: Symlink support varies by operating system and sandbox policy Rejected: Treat every untracked filesystem entry as reviewable source | symlinks can escape the workspace boundary Confidence: high Scope-risk: narrow Reversibility: clean Directive: Event names must describe completed or attempted operations on their real execution path Tested: go test ./internal/agent ./internal/input ./internal/review Not-tested: Windows symlink behavior (tests skip when creation is unavailable)
Rememorio
left a comment
Rememorio
left a comment
There was a problem hiding this comment.
I reviewed the changed lines and left 5 inline comments below. The comments focus on issues worth addressing before merge.
Fresh review found five high-confidence issues: dry-run eagerly provisions Docker, the offline container cannot test typical non-vendored modules, OpenAI-compatible calls have no default timeout, review inputs are unbounded, and the Go fallback disagrees with the Python command-injection rule. Of ten previous comments, seven are fixed and three are partially fixed; remaining work is to surface terminal-write failures, correct staticcheck documentation, and handle untracked files when reviewing from a repository subdirectory.
中文
我看过这次变更的相关代码，在下面留下 5 条行内评论。评论聚焦在合并前值得处理的问题。
本次审查发现 5 个高置信度问题：dry-run 会提前创建 Docker 容器；离线容器无法测试常见的非 vendor Go 模块；OpenAI-compatible 调用默认没有超时；审查输入没有大小上限；Go fallback 的命令注入规则与 Python 实现不一致。10 条历史评论中，7 条已修复、3 条部分修复；剩余工作包括暴露终态写入失败、修正 staticcheck 文档，以及正确处理从仓库子目录发起审查时的 untracked 文件。
| return nil, fmt.Errorf("load skill repository: %w", err) | ||
| } | ||
| // skill_run 和 codeexec 共用同一个执行器。 | ||
| exec, err := execution.NewExecutor(execution.Config{ |
There was a problem hiding this comment.
P1: Dry-run still starts the container runtime
New creates the executor before any request mode is known. With the default container runtime, containerexec.New initializes a Docker container and may pull the image, so --mode dry-run still requires Docker and has container side effects even though the documented contract says it only validates Skill loading and audit flow.
Create the executor and execution tools lazily when an execution-capable path is entered, or provide an executor-free construction path for CLI dry-runs. A factory seam test should prove that dry-run never invokes container creation.
中文
Dry-run 仍会启动容器运行时
New 在获知请求模式之前就创建执行器。使用默认 container runtime 时，containerexec.New 会初始化 Docker 容器，并可能拉取镜像；因此 --mode dry-run 仍依赖 Docker 且会产生容器副作用，与文档中“只验证 Skill 加载与审计链路”的约定不符。
建议只在真正进入执行路径时延迟创建 executor 和相关工具，或者为 CLI dry-run 提供不创建执行器的构造方式。可通过 executor factory 测试确认 dry-run 完全不会触发容器创建。
| return dockercontainer.HostConfig{ | ||
| AutoRemove: true, | ||
| Privileged: false, | ||
| NetworkMode: "none", |
There was a problem hiding this comment.
P1: Offline container lacks Go dependencies
The production container has networking disabled, but only the stock Go image and repository workspace are staged; no module cache is provided. For any non-vendored module with external imports, go test ./... and go vet ./... attempt to download dependencies and fail instead of performing the advertised checks. The current container integration fixture uses only the standard library, so it cannot expose this failure.
Keep network isolation, but stage a vetted read-only module cache, use an image containing the required cache, or explicitly require vendoring and fail early with a clear capability error. Add a container fixture with one external dependency available through the chosen mechanism.
中文
离线容器缺少 Go 依赖来源
生产容器禁用了网络，但当前只提供原始 Go 镜像和仓库工作区，没有预置 module cache。对于包含外部依赖且未提交 vendor 的普通 Go 模块，go test ./... 和 go vet ./... 会尝试下载依赖并直接失败，实际并未执行所宣传的检查。现有容器集成测试只使用标准库，无法暴露这个问题。
建议继续保持网络隔离，同时注入经过校验的只读 module cache、使用预置缓存的镜像，或者明确要求 vendor 并在执行前给出清晰的能力错误。容器测试应加入一个通过所选机制提供的外部依赖。
| // Read 读取或生成 unified diff 输入。 | ||
| func Read(cfg Config, req Request) ([]byte, string, error) { | ||
| if req.DiffFile != "" { | ||
| b, err := os.ReadFile(req.DiffFile) |
There was a problem hiding this comment.
P1: Bound review input before loading it
All supported input paths are loaded into memory without a size limit, including diff files, Git output, file-list contents, and untracked files. A large or hostile input is then copied again while building strings, redacting, parsing hunks, and constructing model requests, so it can exhaust process memory before the output and artifact limits apply.
Add a configurable MaxInputBytes with a safe default and enforce it during reads and Git-output collection rather than after allocation. Cover oversized diff, file-list, and untracked-file inputs and assert that no executor or provider is invoked.
中文
在加载前限制审查输入大小
当前所有输入来源都会在没有大小限制的情况下完整读入内存，包括 diff 文件、Git 输出、文件列表内容和 untracked 文件。之后构造字符串、脱敏、解析 hunk 和生成模型请求时还会产生多份副本，因此超大或恶意输入可能在现有 output/artifact 限制生效前耗尽进程内存。
建议增加带安全默认值的 MaxInputBytes，并在文件读取和 Git 输出收集阶段执行限制，而不是分配完成后才检查。测试应覆盖超大 diff、文件列表和 untracked 文件，并断言 executor 与 Provider 均未被调用。
| if variant != "" { | ||
| opts = append(opts, officialopenai.WithVariant(variant)) | ||
| } | ||
| return officialopenai.New(modelName, opts...), nil |
There was a problem hiding this comment.
P1: Bound OpenAI-compatible model calls
The official OpenAI path constructs its model without an HTTP or request timeout. The underlying default client leaves http.Client.Timeout at zero, and the CLI calls Agent.Run with context.Background(), so a stalled OpenAI-compatible endpoint can keep a review running indefinitely. The generic HTTP provider does not have this behavior because it defaults to 30 seconds.
Pass a bounded timeout through OpenAIConfig, for example with WithHTTPClientOptions(WithHTTPClientTimeout(...)), or derive a deadline around the model step. A stalled-server test should assert the deadline error and terminal task status.
中文
为 OpenAI-compatible 模型调用设置超时
官方 OpenAI 路径创建模型时没有配置 HTTP 或请求超时。底层默认客户端的 http.Client.Timeout 为零，而 CLI 又使用 context.Background() 调用 Agent.Run，因此 OpenAI-compatible 服务端一旦卡住，整次审查可能无限等待。通用 HTTP Provider 默认有 30 秒超时，两条 Provider 路径的行为并不一致。
建议通过 OpenAIConfig 传入有界超时，例如使用 WithHTTPClientOptions(WithHTTPClientTimeout(...))，或在模型步骤外派生带截止时间的 context。补充一个服务端持续阻塞的测试，断言返回超时错误并正确记录任务终态。
| if strings.Contains(text, "\"-c\"") || strings.Contains(text, "'-c'") { | ||
| return true | ||
| } | ||
| if strings.Contains(text, "+") { |
There was a problem hiding this comment.
P2: Fallback flags safe dynamic command arguments
The Go fallback treats any + expression and every non-literal argument to exec.Command as critical command injection. The Python engine only flags a dynamic executable or an explicit shell -c, so safe calls such as exec.Command("git", args...) produce different results depending on whether Python is installed. The existing safe-argument assertion runs only through the default Python path.
Align the fallback with the Python executable-index check, or deliberately apply one shared contract to both engines. Add these safe dynamic-argument cases to the forced-fallback parity test.
中文
Fallback 会误报安全的动态命令参数
Go fallback 会把 exec.Command 中任何包含 + 的表达式以及所有非字面量参数都判定为严重命令注入；Python 实现只检查动态可执行文件和显式的 shell -c。因此 exec.Command("git", args...) 这类安全调用会因环境中是否安装 Python 而得到不同结论。现有安全参数断言只覆盖了默认 Python 路径。
建议让 fallback 与 Python 一样只检查可执行文件所在参数，或者为两个引擎统一采用同一份规则契约，并将这些安全动态参数场景加入强制 fallback 的一致性测试。
Safe dynamic arguments were reported as critical only when Python was unavailable. Match the shared rule contract by evaluating the executable position and explicit shell execution rather than every command argument. Constraint: The Go fallback must produce the same findings as the Python rule engine Rejected: Treat all dynamic command arguments as injection | fixed executables may safely accept dynamic arguments Confidence: high Scope-risk: narrow Reversibility: clean Directive: Maintain forced-fallback parity tests whenever a deterministic rule changes Tested: go test ./internal/review; go test ./internal/rules -run TestRunAllowsFixedExecutableWithDynamicArguments Not-tested: Complex nested command expressions
A stalled compatible endpoint could keep the CLI review running indefinitely because the official provider had neither a client timeout nor a bounded retry policy. Apply the shared 30-second default and disable SDK retries for this bounded review workflow. Constraint: CLI reviews use a background context Rejected: Depend on provider defaults | the SDK client timeout is unbounded Confidence: high Scope-risk: narrow Reversibility: clean Directive: Any provider added to the review path must define a finite request deadline Tested: go test ./internal/llm Not-tested: Real third-party OpenAI-compatible endpoint behavior
Network-isolated containers cannot resolve unvendored Go modules, producing misleading download failures. Preflight the declared vendoring requirement and record an explicit unsupported capability instead. Constraint: Production container networking remains disabled Rejected: Enable network access for go commands | it weakens the sandbox boundary Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep dependency provisioning explicit for every offline runtime Tested: go test ./internal/execution; go test ./internal/agent; go test ./cmd/review-agent Not-tested: Docker integration with a fully vendored third-party dependency
Untrusted diff, file-list, and Git inputs could be copied repeatedly before output limits applied. Enforce a shared input budget during every read and Git command, and preserve repository-relative paths for untracked files reviewed from a subdirectory. Constraint: Input may be controlled by an untrusted local workspace Rejected: Validate size after loading | memory can already be exhausted before the check Confidence: high Scope-risk: moderate Reversibility: clean Directive: Every new review-input source must consume the shared input budget while streaming Tested: go test ./internal/input Not-tested: Extremely large Git stderr output beyond the configured input budget
Dry-run constructed the default container executor before knowing whether code would run, and terminal task-write failures were silently discarded. Defer container construction until execution and preserve persistence failures alongside the original result. Constraint: Container initialization may require Docker and image setup Rejected: Special-case dry-run in the CLI | the Agent API must preserve dry-run semantics for every caller Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep execution resources lazy when a control path can complete without code execution Tested: go test ./internal/execution ./internal/agent Not-tested: Concurrent Close during a real Docker executor initialization
The bounded OpenAI-compatible provider imports the SDK option package directly, but the nested example module still classified the SDK as indirect. Align go.mod with go mod tidy so the examples CI can validate the module. Constraint: Examples CI rejects modules changed by go mod tidy Confidence: high Scope-risk: narrow Reversibility: clean Directive: Update the nested example module whenever a direct import is added Tested: go mod tidy -diff; go test ./internal/llm Not-tested: Full examples module check script
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/code_review_agent/internal/input/input.go (1)
241-261: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
diffFromDirectorydoes not reject symlinks before reading.Line 252's
readFileWithLimit(path, ...)opens whateverentry.Name()resolves to viaos.Open, which follows symlinks. A directory entry that is a symlink to a host file outsiderepoPathwill be read and synthesized into the diff (and can end up in reports or model input). The siblingdiffUntrackedGitFilespath (192-199) already guards this withos.Lstat+info.Mode().IsRegular(); this fallback path should get the same treatment.🛡️ Proposed fix
path := filepath.Join(repoPath, entry.Name()) + if info, err := os.Lstat(path); err != nil || !info.Mode().IsRegular() { + continue + } content, err := readFileWithLimit(path, b.Remaining(), fmt.Sprintf("directory file %q", entry.Name()))🤖 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/input/input.go` around lines 241 - 261, Update diffFromDirectory to inspect each entry with os.Lstat and process only regular files, matching the existing guard in diffUntrackedGitFiles; skip symlinks and other non-regular entries before calling readFileWithLimit.Source: Path instructions
🧹 Nitpick comments (2)
examples/code_review_agent/internal/llm/openai_test.go (1)
121-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTight timing margin may flake under CI load.
The 50ms timeout with a 4x (200ms) elapsed bound leaves little margin for goroutine-scheduling jitter on busy CI runners. Consider a larger base timeout (e.g. 200-300ms) or a more generous multiplier to reduce flake risk while still bounding runaway execution.
As per path instructions for
**/*_test.go, tests should exhibit "deterministic behavior without unnecessary sleeps, randomness, real external services, or machine-specific assumptions."中文
紧凑的时间余量在 CI 负载下可能出现不稳定。
50ms 超时配合 4 倍（200ms）耗时上限，在繁忙的 CI 机器上给协程调度抖动留出的余量很小。建议加大基准超时（例如 200-300ms）或放宽倍数,在保证约束运行时长的同时降低偶发失败风险。
根据路径规则，
**/*_test.go测试应具备"不依赖不必要的 sleep、随机性、真实外部服务或特定机器假设的确定性行为"。🤖 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/llm/openai_test.go` around lines 121 - 193, Increase the timeout margin in TestOpenAIModelProviderTimeoutBoundsStalledRequests by using a larger base timeout and/or a more generous elapsed-time multiplier. Keep the stalled-request simulation and timeout assertions intact while ensuring the bound tolerates normal CI scheduling delays without becoming unbounded.examples/code_review_agent/internal/agent/agent.go (1)
124-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unexporting
ExecutorFactory— it's a test-only seam on a public config.The doc comment says this field exists "for tests," and the only usage in this context is
agent_fast_test.go, which is inpackage agentand can set unexported fields directly. Exporting it puts a test hook permanently on the publicConfigsurface and ties it to an internal-package type (execution.ExecutorFactory).- // ExecutorFactory overrides runtime executor construction for tests. - ExecutorFactory execution.ExecutorFactory + // executorFactory overrides runtime executor construction; test-only seam. + executorFactory execution.ExecutorFactoryAs per path instructions, "whether external consumers need the symbol or it can remain unexported" should be checked for every new export; here the only consumer is a same-package test.
🤖 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/agent/agent.go` around lines 124 - 125, Unexport the test-only Config field currently named ExecutorFactory, renaming it to an unexported form while retaining its execution.ExecutorFactory type and test override behavior. Update agent_fast_test.go and all references in the agent package to use the unexported field, leaving the public Config surface free of this internal test seam.Source: Path instructions
🤖 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/execution/lazy.go`:
- Around line 95-181: Update the closed-while-initializing branch in
LazyExecutor.ensure to run CleanupExecutor(exec) before closing initDone,
capture any cleanup error, and record it in closeErr so Close observes it after
waiting. Adjust LazyExecutor.Close to avoid overwriting an already-recorded
racing cleanup error, while preserving its normal cleanup result when it owned a
non-nil e.exec.
In `@examples/code_review_agent/internal/input/input.go`:
- Around line 465-489: Update runGitCommand to execute git through a context
with a fixed internal timeout, replacing the uncancelled exec.Command/.Run path
and ensuring the context is always released. Preserve the existing stdout/stderr
size limits and error-reporting behavior, and apply the same bounded execution
to gitHeadExists and isGitWorktree if they invoke git directly.
---
Outside diff comments:
In `@examples/code_review_agent/internal/input/input.go`:
- Around line 241-261: Update diffFromDirectory to inspect each entry with
os.Lstat and process only regular files, matching the existing guard in
diffUntrackedGitFiles; skip symlinks and other non-regular entries before
calling readFileWithLimit.
---
Nitpick comments:
In `@examples/code_review_agent/internal/agent/agent.go`:
- Around line 124-125: Unexport the test-only Config field currently named
ExecutorFactory, renaming it to an unexported form while retaining its
execution.ExecutorFactory type and test override behavior. Update
agent_fast_test.go and all references in the agent package to use the unexported
field, leaving the public Config surface free of this internal test seam.
In `@examples/code_review_agent/internal/llm/openai_test.go`:
- Around line 121-193: Increase the timeout margin in
TestOpenAIModelProviderTimeoutBoundsStalledRequests by using a larger base
timeout and/or a more generous elapsed-time multiplier. Keep the stalled-request
simulation and timeout assertions intact while ensuring the bound tolerates
normal CI scheduling delays without becoming unbounded.
🪄 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: b2501e01-eb46-4c6e-826c-79a1b657e0fd
📒 Files selected for processing (17)
examples/code_review_agent/DESIGN.mdexamples/code_review_agent/README.mdexamples/code_review_agent/cmd/review-agent/main.goexamples/code_review_agent/internal/agent/agent.goexamples/code_review_agent/internal/agent/agent_fast_test.goexamples/code_review_agent/internal/agent/input_adapter.goexamples/code_review_agent/internal/agent/sandbox_step.goexamples/code_review_agent/internal/agent/sandbox_step_test.goexamples/code_review_agent/internal/execution/exec.goexamples/code_review_agent/internal/execution/exec_test.goexamples/code_review_agent/internal/execution/lazy.goexamples/code_review_agent/internal/input/input.goexamples/code_review_agent/internal/input/input_test.goexamples/code_review_agent/internal/llm/openai.goexamples/code_review_agent/internal/llm/openai_test.goexamples/code_review_agent/internal/review/skill_test.goexamples/code_review_agent/skills/code-review/scripts/check_fallback.go
🚧 Files skipped from review as they are similar to previous changes (8)
- examples/code_review_agent/internal/agent/input_adapter.go
- examples/code_review_agent/DESIGN.md
- examples/code_review_agent/README.md
- examples/code_review_agent/internal/llm/openai.go
- examples/code_review_agent/cmd/review-agent/main.go
- examples/code_review_agent/internal/review/skill_test.go
- examples/code_review_agent/internal/execution/exec.go
- examples/code_review_agent/skills/code-review/scripts/check_fallback.go
LazyExecutor could signal Close before a concurrently initialized executor had finished cleanup, while discarding cleanup failures. Complete cleanup before releasing waiters and preserve its error for Close callers. Constraint: Container creation can overlap Agent shutdown Rejected: Best-effort cleanup after signaling Close | callers require a completed, error-reporting shutdown contract Confidence: high Scope-risk: narrow Reversibility: clean Directive: Do not release lifecycle waiters before resource cleanup and error aggregation are complete Tested: go test ./internal/execution Not-tested: Real Docker initialization racing Agent.Close
Git discovery and diff commands could stall a review indefinitely despite input-size limits. Run every Git subprocess under a fixed deadline, including HEAD and worktree probes, and propagate deadline failures. Constraint: Input collection accepts arbitrary local repositories Rejected: Rely on caller cancellation alone | input APIs do not yet accept a caller context Confidence: high Scope-risk: narrow Reversibility: clean Directive: Convert to caller-context propagation if the input API gains a context parameter Tested: go test ./internal/input Not-tested: A real Git credential prompt or lock timeout
2 participants
What changed
Added
examples/code_review_agent, a runnable Go code-review Agent example.It accepts unified diffs, repository paths, file lists, and fixtures; runs the
code-reviewSkill; optionally executesgo test,go vet, andstaticcheckin a container sandbox; produces JSON and Markdown reports; and persists review data to SQLite.The example includes deterministic rule-only, dry-run, and fake-model modes, public fixtures, holdout fixtures, report samples, and a Chinese design note.
Why
This demonstrates how tRPC-Agent Go Skills, CodeExecutor, PermissionPolicy, artifacts, telemetry, and SQLite storage can be composed into a verifiable Go-focused CR workflow.
The default runtime is container-based.
local-fallbackis intentionally explicit and intended only for local development and tests. No real model API key is required for the default or fake-model paths.Testing
Also manually ran the CLI against
secret.diffand a fake-model holdout fixture. Both generated reports and SQLite audit records with redacted sensitive values.Notes for reviewers
Please pay particular attention to:
Closes #2004