tool/safety: add tool execution safety guard#2276
Open
shiyudesu wants to merge 3 commits into
Open
Conversation
Add a pre-execution safety guard for command- and code-execution tools. The guard parses each shell command once through internal/shellsafe into a shared analysis result, evaluates deterministic rule families covering dangerous commands, denied paths, network egress, shell bypass, host execution sessions, dependency installation, resource abuse, secret leakage, and environment and working-directory hygiene, and returns an allow, deny, or ask decision before the tool runs. The guard implements tool.PermissionPolicy so the framework blocks denied calls before CallableTool.Call executes, and attaches an after-tool callback that redacts secrets, caps result size, tracks host session lifecycle, and appends correlated preflight and post_execute audit events as JSONL. Policy files in YAML or JSON control allowed and denied commands, denied paths and globs, network domain allowlists, timeouts, output limits, environment whitelists, per-rule actions, and audit behavior without code changes. OpenTelemetry span attributes under tool.safety.* are projected onto the active execute-tool span and mirrored in telemetry/semconv/trace. An example under examples/tool_safety_guard scans a shared corpus of 47 samples and writes tool_safety_report.json and tool_safety_audit.jsonl. Executable quality gates assert at least 90 percent high-risk recall, at most 10 percent safe false positives, 100 percent detection of credential reads, dangerous deletion, and non-whitelisted egress, and a 500-sample scan under one second. Fixes trpc-group#2002
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughEnglishOverviewAdds a configurable Tool Execution Safety Guard in Key capabilities:
Post-execution enforcement and observability
Includes a runnable example ( Public API and compatibilityThis is additive but it changes system behavior only when integrators wire the guard in. New/affected exported Go surface (main symbols):
Compatibility implications to consider
Review questions (export necessity/overlap/semantics)
Behavioral and operational risks
Recommended validation
中文概览在 主要能力：
执行后处理与可观测性
同时加入可运行示例 公共 API 与兼容性这是增量式新增，但只有在集成方把守卫正确接入工具权限链路与回调后才会改变运行行为。 新增/关键导出符号（公共 API 表面）：
兼容性/集成要点
需要重点评审的问题
行为与运行风险
建议验证
WalkthroughThis PR adds a policy-driven ChangesTool Execution Safety Guard
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
I have read the CLA Document and I hereby sign the CLA |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2276 +/- ##
===================================================
+ Coverage 89.85876% 89.96415% +0.10538%
===================================================
Files 1150 1178 +28
Lines 203417 206390 +2973
===================================================
+ Hits 182788 185677 +2889
- Misses 12914 12976 +62
- Partials 7715 7737 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
tool/safety/otel.go (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused
traceimport kept alive only via a dummy variable.
go.opentelemetry.io/otel/trace(Line 13) is imported here only to satisfy the compiler viavar _ = trace.SpanFromContext(Lines 41-42); nothing in this file otherwise usestrace.Span/trace.SpanFromContext. Go imports are file-scoped, so this doesn't "cover"telemetry.go's own (separate) use oftrace— that file already importstraceitself. Removing both the import and the dummy var is safe.🔧 Proposed fix
import ( "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/trace" ) ... - -// Ensure trace.Span is referenced for the imports used by telemetry.go. -var _ = trace.SpanFromContext中文
未使用的
trace导入仅靠一个占位变量来“使用”。第13行导入的
go.opentelemetry.io/otel/trace仅通过第41-42行的var _ = trace.SpanFromContext来避免编译器报“未使用导入”错误，本文件中实际没有任何函数使用trace.Span/trace.SpanFromContext。Go 的导入是按文件生效的，这个占位变量并不能为telemetry.go中独立的trace使用“背书”（该文件本身已自行导入）。可以安全地移除该导入和占位变量（见上方diff）。🤖 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 `@tool/safety/otel.go` around lines 11 - 14, Remove the unused go.opentelemetry.io/otel/trace import and delete the dummy var _ = trace.SpanFromContext declaration in safety-related code; retain the attribute import and all functional behavior unchanged.tool/safety/telemetry.go (1)
87-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate semconv constants across packages, with no test actually enforcing the "canonical" claim.
tool/safetyredeclaresKeyToolSafety*string constants that the PR layer description says also exist as canonical semconv constants intelemetry/semconv/trace/tool_safety.go. The one test meant to guard this relationship only compares against re-hardcoded literals rather than the actual semconv package, so drift between the two declarations would go undetected.
tool/safety/telemetry.go#L87-L105: referencetelemetry/semconv/trace's exported constants directly (if no import cycle) instead of re-declaring the same string values, making one package the single source of truth.tool/safety/telemetry_test.go#L48-L58: once the constants reference (or are imported from)telemetry/semconv/trace, update this test to assert equality against that package's actual exported symbols, not hardcoded string literals, so it truly fails on drift.中文
跨包重复声明 semconv 常量，且测试并未真正强制“权威一致性”。
tool/safety重复声明了KeyToolSafety*字符串常量，而根据PR分层描述，telemetry/semconv/trace/tool_safety.go中也存在等价的权威 semconv 常量。目前唯一用于校验这种关系的测试只是与再次硬编码的字符串比较，而非对照实际的 semconv 包，因此两者之间的漂移无法被发现。
tool/safety/telemetry.go#L87-L105：建议直接引用telemetry/semconv/trace的导出常量（若无循环依赖问题），而非重复声明相同字符串值，使其中一个包成为唯一权威来源。tool/safety/telemetry_test.go#L48-L58：在常量引用/来源统一后，更新该测试以对照该权威包的实际导出符号进行断言，而不是硬编码字符串，从而真正能在出现漂移时失败。🤖 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 `@tool/safety/telemetry.go` around lines 87 - 105, Use the exported constants from telemetry/semconv/trace as the single source of truth instead of redeclaring the KeyToolSafety* constants in tool/safety/telemetry.go, provided this does not introduce an import cycle; update tool/safety/telemetry_test.go to compare against the actual telemetry/semconv/trace symbols rather than hardcoded literals, so drift is detected.Source: Path instructions
tool/safety/artifact_test.go (1)
21-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for the documented Name/URL credential-redaction contract.
redactArtifact's doc comment (artifact.go Lines 23-25) states Name and URL are "always redacted regardless of MIME type because they can carry credentials (e.g.https://user:pass@host/...)". None of the tests here exercise that path — all fixtures only put secrets inData. Since this is explicitly called out as a distinct code path (Lines 38-45 in artifact.go), add a case with a credential-bearingURL/Nameand a non-secretDatato protect this contract from regression.func TestRedactArtifact_URLCredentialsRedactedRegardlessOfMIME(t *testing.T) { in := &artifact.Artifact{ MimeType: "application/octet-stream", URL: "https://user:supersecretpass123@host.example/path", Data: []byte{0x00, 0x01, 0x02}, } out, changed, err := redactArtifact(in) require.NoError(t, err) require.True(t, changed) require.NotContains(t, out.URL, "supersecretpass123") }As per path instructions for
**/*_test.go, tests should "protect a project contract" and prioritize "externally observable behavior" — the URL/Name credential-redaction path is a stated contract without direct coverage.中文
缺少对已文档化的 Name/URL 凭据脱敏契约的测试覆盖。
redactArtifact的文档注释（artifact.go 第23-25行）明确说明 Name 和 URL 无论 MIME 类型如何都会被脱敏，因为它们可能携带凭据（如https://user:pass@host/）。当前测试全部只在Data中放置秘密，未覆盖该独立代码路径（artifact.go 第38-45行）。建议补充一个针对含凭据 URL/Name 的测试用例以保护该契约（示例见上）。根据路径规则，测试应保护项目契约、关注外部可观察行为；而 URL/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 `@tool/safety/artifact_test.go` around lines 21 - 51, Add a test alongside TestRedactArtifact_* that creates an artifact with credential-bearing URL or Name, non-secret binary Data, and a MIME type such as application/octet-stream. Call redactArtifact and assert no error, changed is true, and the credential value is absent from the redacted URL or Name, covering redaction regardless of MIME type.Source: Path instructions
tool/safety/rules_test.go (1)
433-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
scannerWithCustomis dead. It is constructed here but never used — the custom-profile case at Lines 440-447 builds its own scanner, and Line 480 only exists to suppress the unused warning. Remove both to keep the test intent clear.中文
scannerWithCustom是无用代码。 此处创建后从未使用——自定义 profile 用例（第 440-447 行）会自建 scanner，而第 480 行仅用于消除“未使用”告警。建议一并删除，以保持测试意图清晰。🤖 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 `@tool/safety/rules_test.go` around lines 433 - 435, Remove the unused scannerWithCustom construction near the scanner setup and delete the separate suppression reference associated with it; leave the custom-profile test’s own scanner creation and all other test behavior unchanged.Source: Path instructions
tool/safety/decode.go (1)
223-228: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDead guard in
rawIntfloat64 case. Both branches returnint(n), true, so theif n == float64(int(n))check has no effect. If the intent was to reject non-integer JSON numbers (e.g. atimeoutof1.5), this silently truncates instead. Either drop the redundant conditional or reject fractional values explicitly.中文
rawInt的 float64 分支存在无效判断。 两个分支都返回int(n), true，因此if n == float64(int(n))判断毫无作用。如果本意是拒绝非整数 JSON 数字（例如timeout为1.5），当前实现会静默截断。建议删除冗余条件，或显式拒绝小数值。♻️ Proposed simplification
case float64: - if n == float64(int(n)) { - return int(n), true - } - return int(n), true + return int(n), true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tool/safety/decode.go` around lines 223 - 228, Update the float64 case in rawInt to reject fractional values instead of silently truncating them: retain the integer-value conversion path, but return an unsuccessful result when n differs from its integer representation. Remove the redundant branch that currently returns int(n), true in both cases.Source: Path instructions
tool/safety/analysis.go (1)
545-556: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
if/elseinextractNetworkTarget. Both thelocalhostand the else branch sett.Malformed = true, so the conditional is dead. Collapse it (keep the comment) for clarity, or differentiate the behavior iflocalhostwas meant to be handled distinctly.中文
extractNetworkTarget中存在冗余的if/else。localhost分支与 else 分支都设置t.Malformed = true，因此该条件判断是无效的。建议合并（保留注释）以提升可读性；若原意是对localhost做区别处理，则应实现差异化逻辑。🤖 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 `@tool/safety/analysis.go` around lines 545 - 556, In extractNetworkTarget, remove the redundant host == "localhost" conditional because both branches only set t.Malformed = true. Preserve the surrounding explanation as a single comment and retain the same behavior for all hosts without a dot.tool/safety/rules_path.go (1)
205-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
evaluatePathOphelper.
rulePathonly usesevaluatePathOpWithCwd, so this copy never runs and only adds drift risk.中文
删除未使用的
evaluatePathOp辅助函数。
rulePath只调用evaluatePathOpWithCwd，因此这份复制逻辑不会执行，只会增加漂移风险。🤖 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 `@tool/safety/rules_path.go` around lines 205 - 257, Remove the unused evaluatePathOp helper and its entire duplicated finding logic from the path rules implementation. Leave evaluatePathOpWithCwd and the rulePath call flow unchanged, since they provide the active path evaluation behavior.tool/safety/scanner.go (1)
107-119: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
ctxis accepted but never checked for cancellation, especially inScanBatch.Neither
ScannorScanBatchreadsctxat all — none of the 14 rule evaluators receive it, andScanBatch's per-input loop never callsctx.Err(). For a single scan this is low-risk, butScanBatchis explicitly designed for large batches (the perf tests exercise 500 items); a caller that cancels or times out the context has no way to stop an in-flight batch early, wasting CPU on already-abandoned requests.As per path instructions, "Require context.Context to be the first parameter when needed. Check cancellation propagation."
♻️ Suggested cancellation check in ScanBatch
func (s *Scanner) ScanBatch(ctx context.Context, inputs []ScanInput) (BatchReport, error) { batch := BatchReport{ SchemaVersion: "1", GeneratedAt: s.clock(), Reports: make([]ScanReport, 0, len(inputs)), } for _, in := range inputs { + if err := ctx.Err(); err != nil { + return batch, err + } report, err := s.Scan(ctx, in)Also applies to: 192-215
🤖 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 `@tool/safety/scanner.go` around lines 107 - 119, Update ScanBatch to check ctx.Err() before processing each input and stop promptly when the context is canceled or times out, returning the context error. Ensure Scan propagates the context to any rule-evaluation work it performs, and preserve existing scan behavior when ctx remains active.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/tool_safety_guard/README.md`:
- Around line 18-23: Update the expected output block in the README to match
main.go’s CheckToolPermission formatting: show all five case-name lines with
only their decision actions, followed by the existing AfterTool, scan summary,
and report-writing lines. Mark the fenced block as text to satisfy markdownlint
MD040.
In `@tool/safety/analysis.go`:
- Around line 178-190: Ensure the policy’s ConfiguredNetworkCommands reaches
token classification inside analyzeShell before classifyToken runs: either pass
the configured commands into analyzeShell or seed its inner analysis from
buildAnalysis. Preserve the existing mergeAnalysis behavior while making
bare-host arguments for configured downloaders recognized on the shell path.
In `@tool/safety/artifact.go`:
- Around line 101-108: Update the Godoc comment above newArtifactServiceWrapper
to reference the function’s actual unexported name, keeping the constructor
unexported unless external use is explicitly required.
In `@tool/safety/concurrency.go`:
- Around line 85-112: Gate both global counter decrements in the acquire
rollback and release closure on the same MaxActiveCalls > 0 condition that
guards the increment. Update the relevant logic in acquire and its release
callback while preserving the per-tool counter behavior.
In `@tool/safety/guard.go`:
- Around line 403-406: Update the decision handling around stashScanEvent so
scan events are stored only when the request is allowed and will reach the
after-tool callback. Do not call stashScanEvent for denied or ask decisions,
while preserving the existing fromReport data for the allow path.
In `@tool/safety/helpers.go`:
- Around line 209-226: Update parseDecimalInt to use strconv.ParseInt with base
10 and int64 bit size after trimming and validating the empty input, propagating
its overflow or invalid-number error instead of manually accumulating n.
Preserve the existing output assignment and consumed-length behavior for valid
non-negative decimal integers.
In `@tool/safety/redactor.go`:
- Around line 304-332: Update isSecretFieldName so its substring checks also
recognize “token” and “accesskey”, matching the existing exact-match cases and
covering embedded names such as id_token, session_token, and my_access_key.
Preserve the current case-insensitive matching and other secret-name checks.
- Around line 203-270: Update the map[string]any branch in limitWithBudget to
collect and lexicographically sort the map keys before processing entries.
Iterate over the sorted keys while preserving the existing budget checks,
truncation behavior, and value handling so identical inputs produce
deterministic results.
In `@tool/safety/rules_code.go`:
- Around line 267-277: Update the rec.networkCall handling to honor the
extracted URL allowlist before appending the code.network_call Finding, reusing
the existing NetworkTargets/ruleNetwork allowlist logic where applicable.
Suppress the finding when URLs are present and all are allowlisted, while
preserving the current finding behavior for unknown or unavailable targets;
otherwise remove the misleading comment.
In `@tool/safety/rules_command.go`:
- Around line 97-109: Update hasDangerousDelete so rawSourceHasDangerousDelete
is only called when a.Pipeline is nil, indicating parsing failed. After scanning
all successfully parsed pipeline segments without finding danger, return false
directly and preserve the existing nil-analysis and dangerous-segment behavior.
In `@tool/safety/rules_host.go`:
- Around line 163-168: Tighten the fallback matching in the source scan around
the lowercase `src` loop so privilege-command names are recognized only at token
or word boundaries, not anywhere inside quoted or unrelated command text.
Preserve the parsed-argv detection above and ensure benign text such as `echo
"please su to root"` is not classified as `host.privilege`.
In `@tool/safety/session.go`:
- Around line 111-125: Update postExecuteEvent to populate the returned
ScanEvent’s SessionHash with the current session digest, including both the
stashed-event path and the fallback event path, while preserving the existing
Redacted behavior so appendPostExecute receives a non-empty session hash.
---
Nitpick comments:
In `@tool/safety/analysis.go`:
- Around line 545-556: In extractNetworkTarget, remove the redundant host ==
"localhost" conditional because both branches only set t.Malformed = true.
Preserve the surrounding explanation as a single comment and retain the same
behavior for all hosts without a dot.
In `@tool/safety/artifact_test.go`:
- Around line 21-51: Add a test alongside TestRedactArtifact_* that creates an
artifact with credential-bearing URL or Name, non-secret binary Data, and a MIME
type such as application/octet-stream. Call redactArtifact and assert no error,
changed is true, and the credential value is absent from the redacted URL or
Name, covering redaction regardless of MIME type.
In `@tool/safety/decode.go`:
- Around line 223-228: Update the float64 case in rawInt to reject fractional
values instead of silently truncating them: retain the integer-value conversion
path, but return an unsuccessful result when n differs from its integer
representation. Remove the redundant branch that currently returns int(n), true
in both cases.
In `@tool/safety/otel.go`:
- Around line 11-14: Remove the unused go.opentelemetry.io/otel/trace import and
delete the dummy var _ = trace.SpanFromContext declaration in safety-related
code; retain the attribute import and all functional behavior unchanged.
In `@tool/safety/rules_path.go`:
- Around line 205-257: Remove the unused evaluatePathOp helper and its entire
duplicated finding logic from the path rules implementation. Leave
evaluatePathOpWithCwd and the rulePath call flow unchanged, since they provide
the active path evaluation behavior.
In `@tool/safety/rules_test.go`:
- Around line 433-435: Remove the unused scannerWithCustom construction near the
scanner setup and delete the separate suppression reference associated with it;
leave the custom-profile test’s own scanner creation and all other test behavior
unchanged.
In `@tool/safety/scanner.go`:
- Around line 107-119: Update ScanBatch to check ctx.Err() before processing
each input and stop promptly when the context is canceled or times out,
returning the context error. Ensure Scan propagates the context to any
rule-evaluation work it performs, and preserve existing scan behavior when ctx
remains active.
In `@tool/safety/telemetry.go`:
- Around line 87-105: Use the exported constants from telemetry/semconv/trace as
the single source of truth instead of redeclaring the KeyToolSafety* constants
in tool/safety/telemetry.go, provided this does not introduce an import cycle;
update tool/safety/telemetry_test.go to compare against the actual
telemetry/semconv/trace symbols rather than hardcoded literals, so drift is
detected.
🪄 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: e4d72b15-220c-43d9-889b-36b1fdabbb11
📒 Files selected for processing (51)
examples/tool_safety_guard/README.mdexamples/tool_safety_guard/main.goexamples/tool_safety_guard/tool_safety_audit.jsonlexamples/tool_safety_guard/tool_safety_policy.yamlexamples/tool_safety_guard/tool_safety_report.jsoninternal/flow/processor/functioncall_safety_test.gotelemetry/semconv/trace/tool_safety.gotool/safety/analysis.gotool/safety/artifact.gotool/safety/artifact_test.gotool/safety/audit.gotool/safety/audit_test.gotool/safety/benchmark_test.gotool/safety/concurrency.gotool/safety/corpus_test.gotool/safety/decode.gotool/safety/doc.gotool/safety/guard.gotool/safety/guard_test.gotool/safety/helpers.gotool/safety/integration.gotool/safety/otel.gotool/safety/policy.gotool/safety/policy_test.gotool/safety/profile.gotool/safety/quality_test.gotool/safety/redactor.gotool/safety/redactor_test.gotool/safety/rules_code.gotool/safety/rules_command.gotool/safety/rules_dependency.gotool/safety/rules_env_cwd.gotool/safety/rules_host.gotool/safety/rules_metadata.gotool/safety/rules_network.gotool/safety/rules_path.gotool/safety/rules_resource.gotool/safety/rules_secret.gotool/safety/rules_shell.gotool/safety/rules_test.gotool/safety/scanner.gotool/safety/scanner_test.gotool/safety/session.gotool/safety/telemetry.gotool/safety/telemetry_helpers_test.gotool/safety/telemetry_test.gotool/safety/testdata/tool_safety_corpus.jsontool/safety/testdata/tool_safety_policy.yamltool/safety/testhelpers_test.gotool/safety/types.gotool/safety/url.go
- Fix typos (unparseable -> unparsable) flagged by CI - Pass ConfiguredNetworkCommands into shell token classification - Gate global concurrency counter decrement on MaxActiveCalls > 0 - Stash scan events only on the allow path to avoid unbounded growth - Use strconv.ParseInt in parseDecimalInt to reject overflowing sleeps - Sort map keys in limitWithBudget for deterministic truncation - Add token/accesskey substrings to isSecretFieldName - Honor the URL allowlist for code.network_call findings - Restrict raw-source dangerous-delete scan to parse-failure fallback - Match privilege commands at token boundaries in raw-source fallback - Populate SessionHash in postExecuteEvent for post-execute audit - Align example README expected output with actual program output - Add unit tests raising tool/safety coverage from 72% to 98%
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tool/safety/cover_core_guard_test.go`:
- Around line 315-323: Strengthen the pass-through assertions in the
RunAfterTool test so a nil output or nil CustomResult fails explicitly. Require
out to be non-nil, then require out.CustomResult to be non-nil and equal to
"partial", preserving the contract that the unchanged result is returned
untouched.
🪄 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: 71ec1590-e5de-450f-8218-5bcf12f41c18
📒 Files selected for processing (26)
examples/tool_safety_guard/README.mdexamples/tool_safety_guard/tool_safety_audit.jsonlexamples/tool_safety_guard/tool_safety_report.jsontool/safety/analysis.gotool/safety/artifact.gotool/safety/concurrency.gotool/safety/concurrency_test.gotool/safety/cover_core_audit_test.gotool/safety/cover_core_decode_test.gotool/safety/cover_core_guard_test.gotool/safety/cover_core_policy_test.gotool/safety/cover_core_test.gotool/safety/cover_rules_analysis_test.gotool/safety/cover_rules_code_test.gotool/safety/cover_rules_command_test.gotool/safety/cover_rules_redactor_test.gotool/safety/guard.gotool/safety/guard_test.gotool/safety/helpers.gotool/safety/redactor.gotool/safety/redactor_test.gotool/safety/rules_code.gotool/safety/rules_command.gotool/safety/rules_host.gotool/safety/rules_path.gotool/safety/rules_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- examples/tool_safety_guard/README.md
- examples/tool_safety_guard/tool_safety_audit.jsonl
- examples/tool_safety_guard/tool_safety_report.json
- tool/safety/concurrency.go
- tool/safety/rules_test.go
- tool/safety/helpers.go
- tool/safety/rules_host.go
- tool/safety/redactor_test.go
- tool/safety/rules_command.go
- tool/safety/rules_code.go
- tool/safety/rules_path.go
- tool/safety/analysis.go
- tool/safety/artifact.go
- tool/safety/redactor.go
- tool/safety/guard.go
Author
|
Hi @Rememorio @WineChord @Flash-LHR, this PR is ready for review — CI is green, and all CodeRabbit findings have |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Command- and code-execution tools now pass through a pre-execution safety guard that returns an allow, deny, or ask decision before the tool runs. Denied calls are blocked by the framework before tool execution, and an after-tool callback redacts secrets, caps result size, tracks host session lifecycle, and appends correlated preflight and post-execute audit events as JSONL. Behavior is controlled by YAML/JSON policy files (allowed and denied commands, denied paths and globs, network domain allowlists, timeouts, output limits, environment whitelists, per-rule actions, and audit behavior) without code changes, and decisions are exported as
tool.safety.*OpenTelemetry span attributes.Why
Agents that execute shell commands or code can be steered by untrusted input into destructive commands, credential reads, unexpected network egress, dependency installation, or resource abuse. There was no framework-level interception point with a shared, policy-driven decision before tool execution.
The guard parses each command once through
internal/shellsafeinto a shared analysis result so all rule families evaluate the same parse, and implements the existingtool.PermissionPolicyhook so enforcement happens beforeCallableTool.Callrather than inside each tool.Testing
go test -race ./tool/safety— includes corpus quality gates over the shared 47-case corpus (testdata/tool_safety_corpus.json): ≥90% high-risk recall, ≤10% safe false positives, 100% detection of credential reads, dangerous deletion, and non-whitelisted egress, and a 500-sample scan under one secondgo test ./internal/flow/processor -run Safety -count=1cd examples && go vet ./tool_safety_guardexamples/:go run ./tool_safety_guardscans the 19 built-in samples (5 allow / 12 deny / 2 ask) and regeneratestool_safety_report.jsonandtool_safety_audit.jsonlwith output identical to the committed files except timestamps, scan IDs, and durationsNotes for reviewers
tool/safetypackage plustelemetry/semconv/tracespan attribute constants. Labeledtype/api-changeaccordingly.Fixes #2002