tool/safety: add execution safety guard#2249
Conversation
📝 WalkthroughEnglishChange overview
Compatibility and behavioral risks
Recommended validation
中文变更概述
兼容性与行为风险
建议验证步骤
WalkthroughChangesAdds a fail-closed tool safety framework with strict policy parsing, static risk scanning, permission composition, recursive redaction, JSONL auditing, telemetry controls, runtime execution limits, clean environments, process-tree cancellation, built-in tool integration, extensive tests, examples, and bilingual documentation. Tool Safety Framework
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 |
|
All contributors have signed the CLA ✍️ ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2249 +/- ##
===================================================
- Coverage 89.88607% 89.82535% -0.06073%
===================================================
Files 1144 1157 +13
Lines 198637 205491 +6854
===================================================
+ Hits 178547 184583 +6036
- Misses 12588 13085 +497
- Partials 7502 7823 +321
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:
|
|
I have read the CLA Document and I hereby sign the CLA |
0db96e0 to
8d652c5
Compare
There was a problem hiding this comment.
Pull request overview
Adds an opt-in Tool Execution Safety Guard (tool/safety) and wires it through execution entrypoints (workspaceexec/hostexec/codeexec and Graph) to provide fail-closed pre-execution scanning, runtime caps (timeout/output/env), metadata-only auditing, and post-execution result/error redaction—without changing default behavior unless a guard is configured.
Changes:
- Introduce
tool/safetyguard types, strict YAML/JSON policy parsing, auditing sink, redaction utilities, and extensive adversarial/acceptance tests. - Integrate guard enforcement and runtime limits into
workspaceexec,hostexec,codeexec, and Graph execution (including sanitizer barriers and argument suppression in telemetry/events). - Add documentation + scan-only example with sample policy and reference outputs; extend codeexecutor runtimes with hard combined output limits and process-tree cleanup.
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tool/workspaceexec/workspace_exec.go | Adds optional safety guard integration for pre-checks, runtime caps (timeout/output), and post-result sanitization. |
| tool/workspaceexec/workspace_exec_safety_test.go | Adds unit tests for workspaceexec safety argument normalization, profile restriction, and guard behavior. |
| tool/workspaceexec/workspace_exec_policy_test.go | Updates policy tests for new prepareExec signature and adds framework-only guard runtime hardening test. |
| tool/safety/types.go | Defines policy schema, scan/report/audit types, and duration parsing helpers. |
| tool/safety/redact.go | Implements recursive redaction for strings/maps/slices/structs and secret-pattern detection. |
| tool/safety/policy.go | Implements strict YAML/JSON policy parsing, validation, and cloning. |
| tool/safety/policy_test.go | Tests strict parsing, secret rule invariants, duration round-trip, and domain matching. |
| tool/safety/guard_test.go | Adds broad coverage tests for scanning, decision precedence, reload behavior, auditing failures, and sanitizers. |
| tool/safety/audit.go | Adds JSONL audit sink with concurrency safety and owner-only permissions. |
| tool/safety/audit_test.go | Tests JSONL sink correctness, concurrency, permissions, and symlink/directory rejection. |
| tool/safety/adversarial_test.go | Adds adversarial matrices + fuzz/bench for scanner robustness and performance. |
| tool/permission.go | Adds permission policy context plumbing, decision composition helper, and sanitizer interfaces/providers. |
| tool/permission_test.go | Tests decision composition and sanitizer interface contract. |
| tool/hostexec/session.go | Adds max combined output tracking to sessions (and early termination when reached). |
| tool/hostexec/procgroup_windows.go | Uses taskkill /T for Windows process-tree termination with fallback error reporting. |
| tool/hostexec/manager.go | Adds clean-env support and shared output byte budgeting at session creation/runtime. |
| tool/hostexec/hostexec.go | Integrates safety guard with pre-scan, env isolation/scrub for host exec, runtime caps, and sanitization. |
| tool/hostexec/hostexec_safety_test.go | Adds tests for hostexec scanning input shape, blocking, redaction, timeout/output limits, and Windows tree kill. |
| tool/codeexec/codeexec.go | Integrates safety scanning over decoded code blocks, runtime limits propagation, output capping, and sanitization. |
| tool/codeexec/codeexec_safety_test.go | Adds tests for codeexec scanning, limits composition, redaction, and audit failure behavior. |
| tool/callbacks.go | Adds optional panic detail/stack redaction for tool callbacks under safety context. |
| tool/callbacks_test.go | Tests panic recovery redaction behavior. |
| telemetry/trace/span_attribute_policy.go | Adds span attribute keys for tool call arguments/results. |
| telemetry/trace/span_attribute_policy_test.go | Verifies tool call argument/result attribute keys can be dropped via policy. |
| graph/state_graph.go | Ensures permission policies can’t be removed by callbacks; composes decisions; propagates policy context for runtime caps; adds result/error sanitization barriers; suppresses observable args when sanitizers active. |
| graph/state_graph_test.go | Adds tests for policy persistence, sanitizer coverage, decision composition, wrapped tool permission checking, and fail-closed sanitizer errors. |
| examples/tool_safety_guard/tool_safety_policy.yaml | Adds example strict policy for the scan-only demo. |
| examples/tool_safety_guard/sample/tool_safety_audit.jsonl | Provides reference metadata-only audit output for the scan-only demo. |
| examples/tool_safety_guard/README.zh-CN.md | Adds Chinese README for scan-only demo usage and semantics. |
| examples/tool_safety_guard/README.md | Adds English README for scan-only demo usage and semantics. |
| examples/tool_safety_guard/main.go | Implements scan-only demo that validates 16 public samples and emits report + JSONL audit. |
| examples/tool_safety_guard/main_test.go | Tests that demo emits report + metadata-only audit with required schema fields. |
| examples/tool_safety_guard/acceptance_test.go | Adds acceptance-metrics test + benchmark for detection/false-positive rates and performance. |
| examples/tool_safety_guard/.gitignore | Ignores generated demo output directory. |
| docs/tool-safety-guard.zh-CN.md | Adds full Chinese design/usage documentation for the guard. |
| docs/tool-safety-guard.md | Adds full English design/usage documentation for the guard. |
| docs/mkdocs/zh/tool-safety-guard.md | Adds mkdocs Chinese page for the guard. |
| docs/mkdocs/en/tool-safety-guard.md | Adds mkdocs English page for the guard. |
| docs/mkdocs.yml | Adds Tool Safety Guard to mkdocs navigation (EN/ZH). |
| codeexecutor/workspace.go | Adds MaxOutputBytes to RunProgramSpec, OutputLimitReached to RunResult, and capability flag. |
| codeexecutor/local/workspace_runtime.go | Enforces shared stdout/stderr output budget during program execution; adds bounded Wait behavior. |
| codeexecutor/local/workspace_runtime_interactive.go | Adds shared output budget enforcement to interactive sessions and process-group handling tweaks. |
| codeexecutor/local/process_group_windows.go | Adds Windows process group setup and taskkill-based cancel behavior. |
| codeexecutor/local/process_group_unix.go | Adds Unix process group setup and kill-on-cancel behavior (incl. PTY compatibility). |
| codeexecutor/local/output_limit.go | Adds shared output limiter writer used to cancel execution once output budget is reached. |
| codeexecutor/local/output_limit_test.go | Adds tests for output limiting, cancellation timing, and process-tree cleanup. |
| codeexecutor/local/local.go | Propagates clean-env and runtime limits into legacy local code execution path; advertises new capabilities. |
| codeexecutor/local/local_test.go | Adds test asserting clean execution env does not inherit a canary (non-Windows). |
| codeexecutor/codeexecutor.go | Adds context-carried execution limits and clean-env markers for safety-aware executors. |
| codeexecutor/codeexecutor_test.go | Adds test for execution limits context round-trip. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if int64(len(chunk)) >= remaining { | ||
| chunk = chunk[:remaining] | ||
| limitReached = true |
There was a problem hiding this comment.
Thanks for checking this. No code change is needed here: Go permits an expression of any integer type as a slice index, so an int64 index is valid. The package builds and its tests pass with this expression unchanged.
| remaining := s.maxOutputBytes - s.outputBytes | ||
| if int64(len(chunk)) >= remaining { | ||
| chunk = chunk[:remaining] | ||
| s.outputLimitReached = true | ||
| } |
There was a problem hiding this comment.
Thanks for checking this. Go slice indices are not restricted to int; any integer-typed expression, including int64, is valid. This package builds and its tests pass unchanged.
| if remaining <= 0 { | ||
| return "", 0, value != "" | ||
| } | ||
| return value[:remaining], 0, true |
There was a problem hiding this comment.
Thanks for checking this. This is valid Go because slice indices may use any integer type, including int64. The codeexec package builds and tests successfully without a cast.
| CodeBlocks: blocks, | ||
| ExecutionID: aux.ExecutionID, | ||
| } | ||
| guard := effectiveCodeSafetyGuard(ctx, t.cfg.safetyGuard) |
There was a problem hiding this comment.
The guard created here is skipped by the validation returns below, so a secret in language is returned unredacted. Route validation results through the same sanitizer before returning.
中文
这里创建的 guard 会被下方校验返回绕过，导致 `language` 中的密钥未脱敏返回。返回前请让校验结果走同一套 sanitizer。There was a problem hiding this comment.
Fixed in 607608d. Both validation exits now go through finalizeSafetyResult, so direct-guard result/error sanitization is applied consistently. TestExecuteCodeToolSafetyGuardRedactsValidationResult covers a secret embedded in an unsupported language value.
| } | ||
| maxSeconds := int(time.Duration(profile.MaxTimeout) / time.Second) | ||
| if maxSeconds <= 0 { | ||
| return requested |
There was a problem hiding this comment.
Sub-second max_timeout is accepted but ignored here, so hostexec can exceed the profile ceiling. Reject sub-second profiles or enforce a duration timeout.
中文
这里会接受但忽略小于一秒的 `max_timeout`，导致 hostexec 超过 profile 上限。请拒绝小于一秒的 profile，或按 duration 强制超时。There was a problem hiding this comment.
Fixed in 607608d. hostexec now carries the profile ceiling as a time.Duration and execTimeout applies the exact duration, including sub-second limits. The regression test verifies a 500ms ceiling.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tool/hostexec/procgroup_windows.go (1)
32-48: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the
taskkill.execleanup call.The function ignores both
ctxandgrace, then waits indefinitely ontaskkill.exe. If that process stalls, cancellation and session cleanup can hang permanently. Derive a bounded context and useexec.CommandContext.中文
为
taskkill.exe清理调用设置时限。该函数忽略了
ctx和grace，并无限期等待taskkill.exe。一旦该进程卡住，取消和会话清理也会永久阻塞。请创建有期限的 context，并使用exec.CommandContext。🤖 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/hostexec/procgroup_windows.go` around lines 32 - 48, Update terminateProcessTree to honor its ctx and grace parameters by deriving a bounded context for the taskkill.exe invocation, then replace exec.Command with exec.CommandContext using that context. Preserve the existing command arguments and nil-process behavior while ensuring cleanup cannot wait indefinitely.tool/codeexec/codeexec.go (1)
233-241: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize validation results when a direct guard is configured.
The unsupported-language branch returns before scanning and final sanitization. A direct call using
WithSafetyGuardwithlanguage: "password=supersecret"therefore returns that secret verbatim. Route all validation results through the same guarded finalization path.中文
配置直接 Guard 时，也请对校验结果进行脱敏。
不支持语言的分支会在扫描和最终脱敏前直接返回。使用
WithSafetyGuard的直接调用若传入language: "password=supersecret"，会原样返回该秘密。请让所有校验结果经过同一受保护的最终处理路径。🤖 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/codeexec/codeexec.go` around lines 233 - 241, Update the validation flow in the code-block loop within the relevant execution method so missing-code and unsupported-language results are routed through the same final scanning and sanitization path used for normal output. Do not return directly from these validation branches; preserve their existing structured error messages while ensuring WithSafetyGuard redacts sensitive language values before returning.
🟠 Major comments (21)
codeexecutor/local/local.go-187-204 (1)
187-204: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCount error text against the combined output limit.
The error prefix is appended without reducing
remainingOutput, so many failing blocks can make the final result exceedMaxOutputBytes. Route both generated errors and command output through one budget-aware append operation.中文
将错误文本计入统一输出上限。
当前错误前缀不会扣减
remainingOutput，多个失败代码块可使最终结果超过MaxOutputBytes。请让生成的错误信息和命令输出共用同一个限额写入逻辑。🤖 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 `@codeexecutor/local/local.go` around lines 187 - 204, Update the error and successful output handling in the executeCodeBlock loop to use the same budget-aware append operation. Ensure generated error text, blockOutput, and their combined content all decrement remainingOutput and are capped by limits.MaxOutputBytes, while preserving the existing block iteration and error reporting behavior.codeexecutor/local/local.go-175-186 (1)
175-186: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnforce
MaxTimeoutfor direct executor callers.Only
MaxOutputBytesis applied here. A caller usingWithExecutionLimitsdirectly can setMaxTimeout, but execution still uses onlyCodeExecutor.Timeout. Wrap the execution context with the configured limit so this exported contract works independently of the tool wrapper.Proposed fix
limits, limitsEnabled := codeexecutor.ExecutionLimitsFromContext(ctx) + if limitsEnabled && limits.MaxTimeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, limits.MaxTimeout) + defer cancel() + } remainingOutput := limits.MaxOutputBytes中文
为直接调用执行器的场景落实
MaxTimeout。这里仅应用了
MaxOutputBytes。直接通过WithExecutionLimits设置MaxTimeout的调用方仍只受CodeExecutor.Timeout控制。请在执行器内应用该限制，避免安全契约依赖上层工具额外创建 deadline。🤖 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 `@codeexecutor/local/local.go` around lines 175 - 186, Update the execution-context setup in the loop over input.CodeBlocks to apply limits.MaxTimeout for direct executor callers, not only limits.MaxOutputBytes. When a positive MaxTimeout is configured, wrap the block context with the corresponding deadline while preserving the existing output-limit behavior and allowing each block to execute under the configured timeout.codeexecutor/local/process_group_windows.go-28-43 (1)
28-43: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound
taskkilland fall back to killing the root process.
taskkill.execan hang or fail, after whichcmd.Cancelneither returns promptly nor terminates the command. Interactivecmd.Wait()can then outlive its timeout indefinitely. Runtaskkillwith a short context deadline and callcmd.Process.Kill()on failure.中文
请限制
taskkill的执行时间，并在失败时终止根进程。
taskkill.exe一旦卡住或失败，cmd.Cancel将无法及时返回，也不会终止命令，导致交互式cmd.Wait()可能无限超过配置的超时。请为taskkill设置短超时，并在失败时调用cmd.Process.Kill()。As per path instructions, background tasks must be able to stop promptly.
🤖 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 `@codeexecutor/local/process_group_windows.go` around lines 28 - 43, Update the cmd.Cancel function to run taskkill through a short-lived context deadline so it always returns promptly; when taskkill fails or times out, call cmd.Process.Kill() to terminate the root process, while preserving the existing os.ErrProcessDone handling for already-exited processes.Source: Path instructions
tool/hostexec/hostexec_safety_test.go-265-301 (1)
265-301: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSynchronize with the child before testing process-tree termination.
The 200 ms delay does not prove the descendant started, so killing only the parent can pass this test. The later fixed delay can also miss a slow surviving child. Have the helper create a readiness marker, wait with
require.Eventually, terminate the tree, then verify the survivor marker never appears within a bounded interval.中文
测试进程树终止前先与子进程完成同步。
200 毫秒延迟无法证明后代进程已经启动，因此仅终止父进程也可能让测试通过。后续固定延迟同样可能漏掉调度较慢但仍存活的子进程。建议让辅助进程创建就绪标记，使用
require.Eventually等待就绪，再终止进程树，并在限定时间内确认存活标记始终未出现。As per path instructions, tests should be stable, avoid
time.Sleep, and strongly verify cleanup behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tool/hostexec/hostexec_safety_test.go` around lines 265 - 301, Stabilize TestWindowsTerminateProcessTreeKillsDescendant and TestHostExecWindowsProcessTreeHelper by adding a distinct readiness marker that the child writes immediately when it starts, then use require.Eventually to wait for that marker before calling terminateProcessTree. Replace the fixed post-termination sleep with a bounded require.Eventually assertion that the child-survived marker never appears, while preserving the existing process-tree cleanup verification.Source: Path instructions
tool/workspaceexec/workspace_exec.go-1061-1063 (1)
1061-1063: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize failure paths as well as successful stdin results.
Both implementations wrap only the final success return; session lookup, write, and poll failures bypass
SanitizeToolError, breaking the guard’s error-redaction contract.
tool/workspaceexec/workspace_exec.go#L1061-L1063: routegetSessionandsess.proc.Writefailures throughsanitizeSafetyResult.tool/hostexec/hostexec.go#L492-L499: routemgr.writeandmgr.pollfailures throughsanitizeSafetyResult, while preserving cancellation identity.中文
除成功结果外，也要清理 stdin 操作的失败路径。
两个实现都只包装了最终成功返回；会话查询、写入及轮询错误会绕过
SanitizeToolError，破坏 Guard 的错误脱敏契约。
tool/workspaceexec/workspace_exec.go#L1061-L1063：通过sanitizeSafetyResult返回getSession与sess.proc.Write错误。tool/hostexec/hostexec.go#L492-L499：通过sanitizeSafetyResult返回mgr.write与mgr.poll错误，同时保留取消错误的可识别性。As per path instructions, errors must preserve root causes and remain distinguishable to callers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tool/workspaceexec/workspace_exec.go` around lines 1061 - 1063, Update the stdin execution error paths in tool/workspaceexec/workspace_exec.go (lines 1061-1063) so getSession and sess.proc.Write failures are returned through sanitizeSafetyResult, preserving their root causes. In tool/hostexec/hostexec.go (lines 492-499), route mgr.write and mgr.poll failures through sanitizeSafetyResult while preserving cancellation identity and error distinguishability; keep the existing sanitized success return behavior.Source: Path instructions
tool/safety/scan.go-43-44 (1)
43-44: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftNormalize resource durations instead of matching selected spellings.
sleep 2mis denied, but the equivalentsleep 120sdoes not match this regex and is allowed. Parse the duration token so equivalent forms cannot bypassresource_abuse.中文
请规范化资源持续时间，而非仅匹配部分写法。
sleep 2m会被拒绝，但等价的sleep 120s不匹配该正则并会被允许。请解析持续时间参数，避免等价写法绕过resource_abuse。🤖 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/scan.go` around lines 43 - 44, Update the resource-abuse detection around resourcePattern to parse and normalize sleep duration arguments instead of matching only selected unit spellings. Ensure equivalent values such as sleep 2m and sleep 120s are evaluated against the same threshold, while preserving detection of the existing long-duration, infinite-loop, and timeout patterns.tool/safety/scan.go-669-675 (1)
669-675: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftResolve destructive targets before deciding they are safe.
Targets such as
/*and../../..are not classified as protected, sorm -rf /*or a traversal to the filesystem root can be allowed. Resolve relative paths againstWorkingDirand conservatively deny unresolved variables, traversal, or root-level globs.中文
请先解析破坏性目标，再判定其安全性。
/*和../../..不会被识别为受保护目标，因此rm -rf /*或可到达文件系统根目录的路径遍历可能被允许。请基于WorkingDir解析相对路径，并保守拒绝未解析变量、路径遍历和根级通配符。🤖 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/scan.go` around lines 669 - 675, Update protectedRMTarget to resolve destructive targets against WorkingDir before safety classification, including normalizing relative paths such as traversal targets and root-level globs. Conservatively return protected for unresolved variables, traversal that reaches or could reach the filesystem root, and root-level wildcard targets, while preserving the existing protected-path checks.tool/safety/scan.go-64-69 (1)
64-69: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTrack redaction performed on report metadata.
ToolName,Command, orBackendcan be changed byRedactString, butReport.Redactedis derived only from asecret_exposurefinding. The report therefore claims no redaction even when its metadata was redacted.中文
请记录报告元数据中发生的脱敏。
ToolName、Command或Backend可能被RedactString修改，但Report.Redacted仅取决于secret_exposure发现项。因此元数据已脱敏时，报告仍可能错误地声明未脱敏。Also applies to: 195-198
🤖 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/scan.go` around lines 64 - 69, Update the report construction around Report and its RedactString calls so Report.Redacted is set when ToolName, Command, or Backend is changed by redaction, while preserving the existing secret_exposure finding behavior. Reuse the redaction results or otherwise track whether any of these metadata values were redacted, and ensure the final report reflects either metadata redaction or a secret_exposure finding.tool/safety/scan.go-62-73 (1)
62-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRepresent non-allow default actions consistently in reports.
With
DefaultAction=denyoraskand no findings,Blockedbecomes true while the report saysRiskLevel=noneandRecommendation="No action required."Add a synthetic default-policy finding or produce reason/recommendation fields matching the configured decision.中文
请在报告中一致表达非 allow 的默认动作。
当
DefaultAction=deny或ask且没有发现项时，Blocked为 true，但报告却显示RiskLevel=none和No action required.。请增加默认策略发现项，或根据配置的决定生成一致的原因与建议。Also applies to: 178-204
🤖 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/scan.go` around lines 62 - 73, Update scanRequest and the report-generation logic so non-allow p.DefaultAction values remain consistent when no findings exist: either add a synthetic default-policy Finding or derive reason, risk, and recommendation fields from the configured decision. Preserve the existing allow behavior and ensure deny/ask reports do not claim RiskLevel=none or “No action required.” while Blocked is true.tool/safety/scan.go-879-912 (1)
879-912: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound and cycle-protect recursive
RawFieldstraversal.A self-referential
map[string]anyrecurses indefinitely because map/slice traversal neither increments a bounded depth nor tracks visited containers, eventually crashing the process with stack exhaustion. Add a depth limit and cycle detection, then fail closed when the value cannot be fully inspected.中文
请限制
RawFields递归深度并检测循环引用。自引用的
map[string]any会无限递归，因为 map/slice 遍历既没有受限深度，也没有记录已访问容器，最终导致栈耗尽并使进程崩溃。请增加深度限制和循环检测，并在无法完整检查时采用失败关闭策略。🤖 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/scan.go` around lines 879 - 912, Update collectAnyDepth to enforce a maximum traversal depth for nested maps and slices and track visited container identities to detect self-referential cycles. When the depth limit is exceeded or a cycle is encountered, stop traversal and fail closed by rejecting or otherwise marking the value as not fully inspected, while preserving existing scalar extraction and JSON decoding behavior.tool/safety/scan.go-40-40 (1)
40-40: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftCover Git network operations beyond the current verb list.
git ls-remote git@evil.example:org/repo.gitmatches neithergitRemotePattern, a URL, nor another network command, so scanning returns without checking the destination. Global Git options similarly bypass the pattern. Parse Git subcommands conservatively or treat unrecognized remote-capable Git invocations asask.中文
请覆盖当前动词列表之外的 Git 网络操作。
git ls-remote git@evil.example:org/repo.git不匹配gitRemotePattern、URL 或其他网络命令，因此会跳过目标地址检查。Git 全局选项也可绕过该模式。请保守解析 Git 子命令，或将无法识别但可能访问远端的调用判定为ask。Also applies to: 470-479
🤖 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/scan.go` at line 40, Update gitRemotePattern and the surrounding Git command scanning logic to cover remote-capable operations such as ls-remote, including invocations with global Git options before the subcommand. Conservatively parse Git subcommands and ensure unrecognized Git invocations that may access a remote return ask rather than bypassing destination checks; preserve existing handling for known safe and network commands.tool/safety/coverage_test.go-829-850 (1)
829-850: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCover short unquoted credential values.
Add cases such as
password=123andtoken=x; these currently bypass the assignment regex despite having explicit secret labels.中文
请覆盖较短的未加引号凭据值。
请增加
password=123、token=x等场景；尽管键名明确表示机密，当前赋值正则仍会漏掉它们。As per path instructions, sanitizer tests should verify the security boundary.
🤖 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/coverage_test.go` around lines 829 - 850, Add short unquoted assignment cases such as password=123 and token=x to TestRedactStringRecognizesCredentialFamilies, and assert their values are fully redacted while the redacted marker is present. Keep the existing quoted-value and credential-family coverage unchanged.Source: Path instructions
tool/safety/audit_test.go-103-119 (1)
103-119: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftCover path replacement during sink creation.
This only tests a symlink present before
NewJSONLSinkstarts. Add a deterministic hook or platform-specific test proving the target cannot be replaced between inspection and opening.中文
请覆盖创建审计接收器期间的路径替换。
当前仅测试调用前已存在的符号链接。请通过确定性测试钩子或平台测试，验证检查与打开之间无法替换目标。
As per path instructions, external paths must be treated as untrusted.
🤖 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/audit_test.go` around lines 103 - 119, Extend TestJSONLSinkRejectsDirectoryAndSymlink with a deterministic replacement during NewJSONLSink’s inspection-to-open window, using an existing test hook or platform-specific mechanism. Replace the inspected regular file with a symlink before opening and assert NewJSONLSink rejects it, proving untrusted paths cannot be swapped between validation and creation.Source: Path instructions
tool/safety/redact.go-36-38 (1)
36-38: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact short values following explicit secret labels.
tool/safety/redact.go#L36-L38: remove the four-character minimum from unquoted secret assignments.tool/safety/coverage_test.go#L829-L850: add short password, token, and PIN regression cases.中文
请清理明确机密标签后的短值。
tool/safety/redact.go#L36-L38：移除未加引号机密赋值的四字符最低限制。tool/safety/coverage_test.go#L829-L850：增加短密码、令牌及 PIN 的回归测试。As per path instructions, tool output and errors form a security boundary.
🤖 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/redact.go` around lines 36 - 38, Update the unquoted secret-assignment pattern in tool/safety/redact.go, specifically the third regexp in the redaction patterns, to remove the four-character minimum so short values after explicit secret labels are redacted. Add regression cases in tool/safety/coverage_test.go around lines 829-850 covering short passwords, tokens, and PINs.Source: Path instructions
tool/safety/guard.go-327-452 (1)
327-452: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftFail closed on ambiguous or invalid execution fields. Alias collisions are order-dependent, while parsing and overflow errors silently become zero values.
tool/safety/guard.go#L327-L452: return normalization errors for alias conflicts, invalid types, parse failures, and overflow.tool/safety/coverage_test.go#L208-L224: replace the zero-value expectation with an error and restrictive decision assertion.中文
对歧义或无效执行字段采取失败关闭。 别名冲突依赖随机顺序，而解析及溢出错误会静默变成零值。
tool/safety/guard.go#L327-L452：对别名冲突、类型错误、解析失败和溢出返回规范化错误。tool/safety/coverage_test.go#L208-L224：将零值预期改为错误及限制性决策断言。As per path instructions, model-driven tool arguments must be treated as untrusted.
🤖 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/guard.go` around lines 327 - 452, Update tool/safety/guard.go lines 327-452, especially scanRequestFromPermission and the applyPermissionField/stringValue/stringSlice/stringMap/durationValue/int64Value helpers, to fail closed: detect conflicting aliases, reject invalid field types, return parsing errors, and report numeric overflow instead of silently producing zero values; propagate these normalization errors to the caller. Update tool/safety/coverage_test.go lines 208-224 to expect the normalization error and assert the resulting decision is restrictive.Source: Path instructions
tool/safety/audit.go-33-56 (1)
33-56: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSecure the audit path atomically and test path replacement. The current check/open sequence permits a symlink swap in a writable parent directory.
tool/safety/audit.go#L33-L56: use a no-follow, directory-relative secure open and validate the descriptor.tool/safety/audit_test.go#L103-L119: add a regression proving replacement between inspection and open cannot redirect the sink.中文
请以原子方式安全打开审计路径，并测试路径替换。 当前检查与打开分离，可在可写父目录中被替换为符号链接。
tool/safety/audit.go#L33-L56：使用禁止跟随符号链接的目录相对安全打开，并验证文件描述符。tool/safety/audit_test.go#L103-L119：增加检查与打开之间路径替换无法重定向接收器的回归测试。As per path instructions, external paths must be treated as untrusted.
🤖 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/audit.go` around lines 33 - 56, Atomically secure the audit path in the audit-file opening flow in tool/safety/audit.go:33-56 by replacing the separate path inspection and open sequence with a directory-relative, no-follow secure open, then validate the opened descriptor remains a regular file and apply the required permissions. Add a regression test in tool/safety/audit_test.go:103-119 that replaces the target between inspection and open and verifies the audit sink cannot be redirected through a symlink.Source: Path instructions
tool/safety/guard.go-274-299 (1)
274-299: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not hide control-flow errors behind an unwrap-only sanitizer.
tool/safety/guard.go#L274-L299: preserve the identity required by direct interrupt helpers while separately sanitizing user-visible text.tool/safety/coverage_test.go#L785-L827: test the established interrupt helper directly, not onlyerrors.Is.中文
不要用仅支持解包的清理包装隐藏控制流错误。
tool/safety/guard.go#L274-L299：保留直接中断辅助函数所需的错误身份，同时单独清理用户可见文本。tool/safety/coverage_test.go#L785-L827：直接测试现有中断辅助函数，而不仅是errors.Is。Based on learnings, interrupt helpers in this repository are intentionally non-unwrap-aware.
🤖 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/guard.go` around lines 274 - 299, Update tool/safety/guard.go lines 274-299 in Guard.SanitizeToolError to preserve the original control-flow error identity for direct interrupt helpers while providing separately redacted user-visible text; do not rely solely on an unwrap-based sanitizedToolError. Update tool/safety/coverage_test.go lines 785-827 to invoke and assert the established interrupt helper directly, while still verifying the redacted message.Source: Learnings
graph/state_graph.go-4347-4353 (1)
4347-4353: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnable panic redaction before running Graph callbacks.
Unlike the function-call pipeline, this Graph path invokes before/after callbacks without
WithRedactedToolCallbackPanics. A guarded callback panic is therefore logged with its raw value and stack before the final error sanitizer runs. Install the marker whenever either effective sanitizer is active and preserve it across callback-returned contexts.中文
在 Graph 回调执行前启用 panic 脱敏。
与函数调用流水线不同，此 Graph 路径在执行前置/后置回调前没有设置
WithRedactedToolCallbackPanics。因此，即使启用了 Guard，回调 panic 的原始值和堆栈仍会在最终错误净化前写入日志。任一有效净化器存在时都应设置该标记，并在回调返回新 context 后重新应用。As per path instructions, guarded Graph execution must keep callback diagnostics within the same security boundary.
🤖 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 `@graph/state_graph.go` around lines 4347 - 4353, Update the Graph callback execution flow around startInvocation and policyInvocation to apply WithRedactedToolCallbackPanics whenever either effective panic sanitizer is enabled, before invoking callbacks. Reapply the marker to any context returned by before/after callbacks so callback diagnostics remain redacted across context replacement, while preserving the existing sanitizer and callback behavior.Source: Path instructions
internal/flow/processor/functioncall.go-1123-1125 (1)
1123-1125: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winHonor error-only sanitizers when redacting diagnostics.
ToolResultSanitizerandToolErrorSanitizerare independent, but these branches redact only when a result sanitizer exists. A valid error-only policy can therefore leak panic details, arguments, results, or stream errors. Gate diagnostics on either sanitizer—or the existing redaction context marker.
internal/flow/processor/functioncall.go#L1123-L1125: redact panic details when either sanitizer applies.internal/flow/processor/functioncall.go#L1900-L1914: hide arguments for either sanitizer.internal/flow/processor/functioncall.go#L3110-L3127: use the shared redaction marker for failure logs.internal/flow/processor/functioncall.go#L3542-L3550: recognize error-only sanitizers for stream errors.internal/flow/processor/functioncall_test.go#L163-L208: add a policy implementing onlyToolErrorSanitizer.中文
脱敏诊断信息时应支持仅实现错误净化器的策略。
ToolResultSanitizer与ToolErrorSanitizer是独立接口，但这些分支仅在结果净化器存在时脱敏。合法的“仅错误净化”策略仍可能泄露 panic、参数、结果或流错误内容。应在任一净化器存在时脱敏，或统一使用现有的脱敏 context 标记。
internal/flow/processor/functioncall.go#L1123-L1125：任一净化器存在时隐藏 panic 详情。internal/flow/processor/functioncall.go#L1900-L1914：任一净化器存在时隐藏参数。internal/flow/processor/functioncall.go#L3110-L3127：失败日志使用统一脱敏标记。internal/flow/processor/functioncall.go#L3542-L3550：流错误应识别仅错误净化器。internal/flow/processor/functioncall_test.go#L163-L208：增加仅实现ToolErrorSanitizer的策略测试。As per path instructions, independently exposed sanitizer capabilities must retain their security guarantees.
🤖 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 `@internal/flow/processor/functioncall.go` around lines 1123 - 1125, Honor error-only sanitizers across all diagnostic paths: in internal/flow/processor/functioncall.go lines 1123-1125, 1900-1914, 3110-3127, and 3542-3550, redact panic details and arguments when either sanitizer applies, use the shared redaction context marker for failure logs, and recognize error-only policies for stream errors. In internal/flow/processor/functioncall_test.go lines 163-208, add coverage for a policy implementing only ToolErrorSanitizer.Source: Path instructions
graph/state_graph.go-6036-6061 (1)
6036-6061: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftCompose every applicable sanitizer instead of selecting one.
The invocation sanitizer currently shadows the tool-local sanitizer, while batched calls use only the first discovered sanitizer. This can skip tool-specific redaction, auditing, or custom containment. Apply all applicable sanitizers deterministically and fail closed on any failure.
graph/state_graph.go#L6036-L6061: compose invocation and semantic-tool result/error sanitizers.internal/flow/processor/functioncall.go#L2109-L2131: compose invocation and tool-local sanitizers.internal/flow/processor/functioncall.go#L2219-L2249: sanitize batched hook outputs per call instead of returning the first sanitizer.中文
组合所有适用的净化器，而不是只选择一个。
当前运行级净化器会覆盖工具本地净化器，批量调用则只使用找到的第一个净化器。这可能跳过工具专属脱敏、审计或 containment 逻辑。请按确定顺序应用全部净化器，并在任一失败时关闭输出。
graph/state_graph.go#L6036-L6061：组合运行级与语义工具的结果/错误净化器。internal/flow/processor/functioncall.go#L2109-L2131：组合运行级与工具本地净化器。internal/flow/processor/functioncall.go#L2219-L2249：按调用分别净化批量 hook 输出，而非采用首个净化器。As per path instructions, framework extension points must remain capability-oriented and composable.
🤖 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 `@graph/state_graph.go` around lines 6036 - 6061, Compose every applicable sanitizer in deterministic order rather than selecting only one, and fail closed if any sanitizer fails. In graph/state_graph.go lines 6036-6061, update effectiveGraphToolResultSanitizer and effectiveGraphToolErrorSanitizer to combine invocation and semantic-tool sanitizers. In internal/flow/processor/functioncall.go lines 2109-2131, compose invocation and tool-local sanitizers; in lines 2219-2249, sanitize each batched hook output using the sanitizers for that specific call instead of the first discovered sanitizer.Source: Path instructions
internal/flow/processor/functioncall.go-583-587 (1)
583-587: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve no-Guard
AfterToolMessagesmutation behavior.The event is now cloned unconditionally, so plugins that previously mutated
ToolResultEventin place silently stop working even when no Guard is configured. Clone only when sanitization requires isolation, or re-sanitize accepted mutations afterward.
internal/flow/processor/functioncall.go#L583-L587: preserve the original event for no-Guard runs.internal/flow/processor/functioncall_test.go#L2804-L2831: add a no-Guard compatibility case proving existing in-place mutations still apply.中文
保留未配置 Guard 时的
AfterToolMessages修改语义。当前无条件克隆事件，导致原先通过原地修改
ToolResultEvent的插件即使未配置 Guard 也会静默失效。仅在净化隔离确有需要时克隆，或在接受修改后再次执行最终净化。
internal/flow/processor/functioncall.go#L583-L587：未配置 Guard 时继续传递原始事件。internal/flow/processor/functioncall_test.go#L2804-L2831：增加未配置 Guard 的兼容性测试。As per path instructions, opt-in framework safety must preserve existing default behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/flow/processor/functioncall.go` around lines 583 - 587, Update the event construction in internal/flow/processor/functioncall.go:583-587 around ToolResultEvent and the Guard sanitization flow so no-Guard runs pass the original event and preserve in-place AfterToolMessages mutations; only clone when sanitization requires isolation, or reapply accepted mutations after sanitization. Add a no-Guard compatibility test in internal/flow/processor/functioncall_test.go:2804-2831 proving plugin mutations to ToolResultEvent remain effective.Source: Path instructions
🟡 Minor comments (2)
tool/safety/guard_test.go-236-244 (1)
236-244: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAssert the concurrent scan outcomes during reload.
The goroutines discard both the report and error, so the test still passes if readers observe a transient invalid snapshot. Collect the errors and assert every scan succeeds.
中文
请断言策略重载期间并发扫描的结果。
当前协程丢弃了报告和错误；即使读取方短暂观察到无效快照，测试仍会通过。请收集错误并断言所有扫描均成功。
As per path instructions, concurrency tests should cover error propagation and consistency.
🤖 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/guard_test.go` around lines 236 - 244, Update the concurrent scan test around guard.Scan and the sync.WaitGroup to collect each goroutine’s error safely, then after wg.Wait assert that every scan completed without error. Preserve the concurrent Reload call and ensure the assertions detect any transient invalid snapshot or error propagation failure.Source: Path instructions
docs/tool-safety-guard.zh-CN.md-37-37 (1)
37-37: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSurface
JSONLSink.Closefailures in the sample.
Closeperforms the final sync and can report audit durability failures.defer sink.Close()discards that error, so copied code may report success without durably finalizing the audit trail. Handle the deferred error or close explicitly during shutdown.中文
示例应处理
JSONLSink.Close错误。
Close会执行最终同步，并可能返回审计持久化错误。defer sink.Close()会丢弃该错误，导致应用在审计记录未可靠落盘时仍报告成功。请合并延迟关闭错误，或在关闭流程中显式处理。As per path instructions, documentation examples must accurately reflect the executable audit contract.
🤖 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 `@docs/tool-safety-guard.zh-CN.md` at line 37, Update the documentation sample’s shutdown flow around JSONLSink.Close so its returned error is handled rather than discarded by defer sink.Close(). Preserve deferred cleanup if needed, but capture and propagate or report the close failure so the sample cannot indicate successful completion when audit finalization fails.Source: Path instructions
🧹 Nitpick comments (2)
codeexecutor/local/output_limit_test.go (1)
110-110: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftReplace the fixed sleeps with deterministic process synchronization.
A loaded runner can delay the child beyond 1.5 seconds, letting these tests pass before a surviving descendant writes the marker. Coordinate child startup and assert its termination directly instead.
中文
请用确定性的进程同步替代固定休眠。
高负载环境可能让子进程延迟超过 1.5 秒，导致测试在仍有后代进程存活时提前通过。请同步确认子进程已启动，并直接验证其终止。
As per path instructions, tests must remain stable and avoid
time.Sleepor machine-specific timing.Also applies to: 135-135
🤖 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 `@codeexecutor/local/output_limit_test.go` at line 110, Replace the fixed time.Sleep calls in the output-limit tests with deterministic process synchronization: wait for an explicit child-start signal, then directly wait for and assert the child process terminates before checking the marker outcome. Keep the tests free of time-based or machine-dependent timing assumptions.Source: Path instructions
tool/callbacks_test.go (1)
148-162: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that panic logs are redacted too.
This only verifies the returned error. Capture the logger and assert that neither
supersecretnor the raw panic stack appears, covering the security-sensitive logging branch added intool/callbacks.go.中文
同时验证 panic 日志已脱敏。
当前测试只验证返回错误。请捕获日志，并断言其中不包含
supersecret或原始 panic 堆栈，以覆盖tool/callbacks.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 `@tool/callbacks_test.go` around lines 148 - 162, Extend TestRunToolResultMessages_PanicRecoveryRedactsSafetyContext to capture the logger output while running the panic callback, then assert the logged content contains neither “supersecret” nor the raw panic stack. Preserve the existing returned-error assertions and ensure the test exercises the redacted panic logging path in RunToolResultMessages.
🤖 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/hostexec/hostexec.go`:
- Around line 723-738: Update safetyExecutionEnv to merge trusted baseEnv
separately from caller-controlled extra, preserving trusted runtime environment
values. Reject case-insensitive overrides of PATH, SYSTEMROOT, COMSPEC, and
other protected runtime keys from extra before applying envscrub, while
retaining allowed request variables.
---
Outside diff comments:
In `@tool/codeexec/codeexec.go`:
- Around line 233-241: Update the validation flow in the code-block loop within
the relevant execution method so missing-code and unsupported-language results
are routed through the same final scanning and sanitization path used for normal
output. Do not return directly from these validation branches; preserve their
existing structured error messages while ensuring WithSafetyGuard redacts
sensitive language values before returning.
In `@tool/hostexec/procgroup_windows.go`:
- Around line 32-48: Update terminateProcessTree to honor its ctx and grace
parameters by deriving a bounded context for the taskkill.exe invocation, then
replace exec.Command with exec.CommandContext using that context. Preserve the
existing command arguments and nil-process behavior while ensuring cleanup
cannot wait indefinitely.
---
Major comments:
In `@codeexecutor/local/local.go`:
- Around line 187-204: Update the error and successful output handling in the
executeCodeBlock loop to use the same budget-aware append operation. Ensure
generated error text, blockOutput, and their combined content all decrement
remainingOutput and are capped by limits.MaxOutputBytes, while preserving the
existing block iteration and error reporting behavior.
- Around line 175-186: Update the execution-context setup in the loop over
input.CodeBlocks to apply limits.MaxTimeout for direct executor callers, not
only limits.MaxOutputBytes. When a positive MaxTimeout is configured, wrap the
block context with the corresponding deadline while preserving the existing
output-limit behavior and allowing each block to execute under the configured
timeout.
In `@codeexecutor/local/process_group_windows.go`:
- Around line 28-43: Update the cmd.Cancel function to run taskkill through a
short-lived context deadline so it always returns promptly; when taskkill fails
or times out, call cmd.Process.Kill() to terminate the root process, while
preserving the existing os.ErrProcessDone handling for already-exited processes.
In `@graph/state_graph.go`:
- Around line 4347-4353: Update the Graph callback execution flow around
startInvocation and policyInvocation to apply WithRedactedToolCallbackPanics
whenever either effective panic sanitizer is enabled, before invoking callbacks.
Reapply the marker to any context returned by before/after callbacks so callback
diagnostics remain redacted across context replacement, while preserving the
existing sanitizer and callback behavior.
- Around line 6036-6061: Compose every applicable sanitizer in deterministic
order rather than selecting only one, and fail closed if any sanitizer fails. In
graph/state_graph.go lines 6036-6061, update effectiveGraphToolResultSanitizer
and effectiveGraphToolErrorSanitizer to combine invocation and semantic-tool
sanitizers. In internal/flow/processor/functioncall.go lines 2109-2131, compose
invocation and tool-local sanitizers; in lines 2219-2249, sanitize each batched
hook output using the sanitizers for that specific call instead of the first
discovered sanitizer.
In `@internal/flow/processor/functioncall.go`:
- Around line 1123-1125: Honor error-only sanitizers across all diagnostic
paths: in internal/flow/processor/functioncall.go lines 1123-1125, 1900-1914,
3110-3127, and 3542-3550, redact panic details and arguments when either
sanitizer applies, use the shared redaction context marker for failure logs, and
recognize error-only policies for stream errors. In
internal/flow/processor/functioncall_test.go lines 163-208, add coverage for a
policy implementing only ToolErrorSanitizer.
- Around line 583-587: Update the event construction in
internal/flow/processor/functioncall.go:583-587 around ToolResultEvent and the
Guard sanitization flow so no-Guard runs pass the original event and preserve
in-place AfterToolMessages mutations; only clone when sanitization requires
isolation, or reapply accepted mutations after sanitization. Add a no-Guard
compatibility test in internal/flow/processor/functioncall_test.go:2804-2831
proving plugin mutations to ToolResultEvent remain effective.
In `@tool/hostexec/hostexec_safety_test.go`:
- Around line 265-301: Stabilize TestWindowsTerminateProcessTreeKillsDescendant
and TestHostExecWindowsProcessTreeHelper by adding a distinct readiness marker
that the child writes immediately when it starts, then use require.Eventually to
wait for that marker before calling terminateProcessTree. Replace the fixed
post-termination sleep with a bounded require.Eventually assertion that the
child-survived marker never appears, while preserving the existing process-tree
cleanup verification.
In `@tool/safety/audit_test.go`:
- Around line 103-119: Extend TestJSONLSinkRejectsDirectoryAndSymlink with a
deterministic replacement during NewJSONLSink’s inspection-to-open window, using
an existing test hook or platform-specific mechanism. Replace the inspected
regular file with a symlink before opening and assert NewJSONLSink rejects it,
proving untrusted paths cannot be swapped between validation and creation.
In `@tool/safety/audit.go`:
- Around line 33-56: Atomically secure the audit path in the audit-file opening
flow in tool/safety/audit.go:33-56 by replacing the separate path inspection and
open sequence with a directory-relative, no-follow secure open, then validate
the opened descriptor remains a regular file and apply the required permissions.
Add a regression test in tool/safety/audit_test.go:103-119 that replaces the
target between inspection and open and verifies the audit sink cannot be
redirected through a symlink.
In `@tool/safety/coverage_test.go`:
- Around line 829-850: Add short unquoted assignment cases such as password=123
and token=x to TestRedactStringRecognizesCredentialFamilies, and assert their
values are fully redacted while the redacted marker is present. Keep the
existing quoted-value and credential-family coverage unchanged.
In `@tool/safety/guard.go`:
- Around line 327-452: Update tool/safety/guard.go lines 327-452, especially
scanRequestFromPermission and the
applyPermissionField/stringValue/stringSlice/stringMap/durationValue/int64Value
helpers, to fail closed: detect conflicting aliases, reject invalid field types,
return parsing errors, and report numeric overflow instead of silently producing
zero values; propagate these normalization errors to the caller. Update
tool/safety/coverage_test.go lines 208-224 to expect the normalization error and
assert the resulting decision is restrictive.
- Around line 274-299: Update tool/safety/guard.go lines 274-299 in
Guard.SanitizeToolError to preserve the original control-flow error identity for
direct interrupt helpers while providing separately redacted user-visible text;
do not rely solely on an unwrap-based sanitizedToolError. Update
tool/safety/coverage_test.go lines 785-827 to invoke and assert the established
interrupt helper directly, while still verifying the redacted message.
In `@tool/safety/redact.go`:
- Around line 36-38: Update the unquoted secret-assignment pattern in
tool/safety/redact.go, specifically the third regexp in the redaction patterns,
to remove the four-character minimum so short values after explicit secret
labels are redacted. Add regression cases in tool/safety/coverage_test.go around
lines 829-850 covering short passwords, tokens, and PINs.
In `@tool/safety/scan.go`:
- Around line 43-44: Update the resource-abuse detection around resourcePattern
to parse and normalize sleep duration arguments instead of matching only
selected unit spellings. Ensure equivalent values such as sleep 2m and sleep
120s are evaluated against the same threshold, while preserving detection of the
existing long-duration, infinite-loop, and timeout patterns.
- Around line 669-675: Update protectedRMTarget to resolve destructive targets
against WorkingDir before safety classification, including normalizing relative
paths such as traversal targets and root-level globs. Conservatively return
protected for unresolved variables, traversal that reaches or could reach the
filesystem root, and root-level wildcard targets, while preserving the existing
protected-path checks.
- Around line 64-69: Update the report construction around Report and its
RedactString calls so Report.Redacted is set when ToolName, Command, or Backend
is changed by redaction, while preserving the existing secret_exposure finding
behavior. Reuse the redaction results or otherwise track whether any of these
metadata values were redacted, and ensure the final report reflects either
metadata redaction or a secret_exposure finding.
- Around line 62-73: Update scanRequest and the report-generation logic so
non-allow p.DefaultAction values remain consistent when no findings exist:
either add a synthetic default-policy Finding or derive reason, risk, and
recommendation fields from the configured decision. Preserve the existing allow
behavior and ensure deny/ask reports do not claim RiskLevel=none or “No action
required.” while Blocked is true.
- Around line 879-912: Update collectAnyDepth to enforce a maximum traversal
depth for nested maps and slices and track visited container identities to
detect self-referential cycles. When the depth limit is exceeded or a cycle is
encountered, stop traversal and fail closed by rejecting or otherwise marking
the value as not fully inspected, while preserving existing scalar extraction
and JSON decoding behavior.
- Line 40: Update gitRemotePattern and the surrounding Git command scanning
logic to cover remote-capable operations such as ls-remote, including
invocations with global Git options before the subcommand. Conservatively parse
Git subcommands and ensure unrecognized Git invocations that may access a remote
return ask rather than bypassing destination checks; preserve existing handling
for known safe and network commands.
In `@tool/workspaceexec/workspace_exec.go`:
- Around line 1061-1063: Update the stdin execution error paths in
tool/workspaceexec/workspace_exec.go (lines 1061-1063) so getSession and
sess.proc.Write failures are returned through sanitizeSafetyResult, preserving
their root causes. In tool/hostexec/hostexec.go (lines 492-499), route mgr.write
and mgr.poll failures through sanitizeSafetyResult while preserving cancellation
identity and error distinguishability; keep the existing sanitized success
return behavior.
---
Minor comments:
In `@docs/tool-safety-guard.zh-CN.md`:
- Line 37: Update the documentation sample’s shutdown flow around
JSONLSink.Close so its returned error is handled rather than discarded by defer
sink.Close(). Preserve deferred cleanup if needed, but capture and propagate or
report the close failure so the sample cannot indicate successful completion
when audit finalization fails.
In `@tool/safety/guard_test.go`:
- Around line 236-244: Update the concurrent scan test around guard.Scan and the
sync.WaitGroup to collect each goroutine’s error safely, then after wg.Wait
assert that every scan completed without error. Preserve the concurrent Reload
call and ensure the assertions detect any transient invalid snapshot or error
propagation failure.
---
Nitpick comments:
In `@codeexecutor/local/output_limit_test.go`:
- Line 110: Replace the fixed time.Sleep calls in the output-limit tests with
deterministic process synchronization: wait for an explicit child-start signal,
then directly wait for and assert the child process terminates before checking
the marker outcome. Keep the tests free of time-based or machine-dependent
timing assumptions.
In `@tool/callbacks_test.go`:
- Around line 148-162: Extend
TestRunToolResultMessages_PanicRecoveryRedactsSafetyContext to capture the
logger output while running the panic callback, then assert the logged content
contains neither “supersecret” nor the raw panic stack. Preserve the existing
returned-error assertions and ensure the test exercises the redacted panic
logging path in RunToolResultMessages.
🪄 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: 064d9ae4-606d-42d1-b3c1-0394f94d73d2
📒 Files selected for processing (56)
codeexecutor/codeexecutor.gocodeexecutor/codeexecutor_test.gocodeexecutor/local/local.gocodeexecutor/local/local_test.gocodeexecutor/local/output_limit.gocodeexecutor/local/output_limit_test.gocodeexecutor/local/process_group_unix.gocodeexecutor/local/process_group_windows.gocodeexecutor/local/workspace_runtime.gocodeexecutor/local/workspace_runtime_interactive.gocodeexecutor/workspace.godocs/mkdocs.ymldocs/mkdocs/en/tool-safety-guard.mddocs/mkdocs/zh/tool-safety-guard.mddocs/tool-safety-guard.mddocs/tool-safety-guard.zh-CN.mdexamples/tool_safety_guard/.gitignoreexamples/tool_safety_guard/README.mdexamples/tool_safety_guard/README.zh-CN.mdexamples/tool_safety_guard/acceptance_test.goexamples/tool_safety_guard/main.goexamples/tool_safety_guard/main_test.goexamples/tool_safety_guard/sample/tool_safety_audit.jsonlexamples/tool_safety_guard/sample/tool_safety_report.jsonexamples/tool_safety_guard/tool_safety_policy.yamlgraph/state_graph.gograph/state_graph_test.gointernal/flow/processor/functioncall.gointernal/flow/processor/functioncall_test.gotelemetry/trace/span_attribute_policy.gotelemetry/trace/span_attribute_policy_test.gotool/callbacks.gotool/callbacks_test.gotool/codeexec/codeexec.gotool/codeexec/codeexec_safety_test.gotool/hostexec/hostexec.gotool/hostexec/hostexec_safety_test.gotool/hostexec/manager.gotool/hostexec/procgroup_windows.gotool/hostexec/session.gotool/permission.gotool/permission_test.gotool/safety/adversarial_test.gotool/safety/audit.gotool/safety/audit_test.gotool/safety/coverage_test.gotool/safety/guard.gotool/safety/guard_test.gotool/safety/policy.gotool/safety/policy_test.gotool/safety/redact.gotool/safety/scan.gotool/safety/types.gotool/workspaceexec/workspace_exec.gotool/workspaceexec/workspace_exec_policy_test.gotool/workspaceexec/workspace_exec_safety_test.go
8d652c5 to
3f14b4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tool/safety/coverage_test.go (2)
289-293: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise nested slices returned by
Guard.Policy().Replacing a map entry only verifies map isolation. Mutate each returned profile slice and confirm the active guard remains unchanged; otherwise a shallow copy could permit unvalidated policy mutation.
中文
请验证
Guard.Policy()返回值中的嵌套切片也已复制。当前仅替换 map 条目，只能验证 map 隔离。应修改返回 profile 的各切片并确认 Guard 内部策略不变，否则浅拷贝可能允许绕过校验修改策略。
As per path instructions, assertions must strongly validate externally observable safety contracts.
🤖 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/coverage_test.go` around lines 289 - 293, Extend the defensive-copy test around Guard.Policy() to mutate each nested slice in the returned exec ToolProfile, including AllowedCommands, and verify the active guard policy retains its original values. Keep the existing map-entry replacement assertion, and ensure the checks detect shallow-copy leakage across every profile slice exposed by ToolProfile.Source: Path instructions
1091-1097: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid pinning the number and order of findings.
len(report.Findings) == 2andFindings[0]couple the test to detector internals. Locate the relevant rule by ID and assert its action so adding another valid detector does not break compatibility tests.中文
避免固定 findings 的数量和顺序。
len(report.Findings) == 2和Findings[0]将测试绑定到检测器内部实现。请按规则 ID 查找目标 finding 并验证其动作，避免新增合法检测规则时产生无行为回归的测试失败。As per path instructions, tests should validate external behavior rather than implementation details.
🤖 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/coverage_test.go` around lines 1091 - 1097, Update the assertions in the guard.Scan test for Command "rm -rf /" to stop requiring exactly two findings or relying on Findings[0]. Locate the relevant custom-rule finding by its rule ID, assert that it has PermissionActionAsk, and retain the existing external-behavior checks for the report decision, blocked status, reason, and recommendation.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 `@docs/tool-safety-guard.zh-CN.md`:
- Line 115: 更新文档中关于 OpenTelemetry 属性的表述：将“低基数”改为“数量有界且已净化”，并明确说明
tool.safety.request_sha256 是可能高基数、用于请求关联的属性；保留其余属性和可丢弃原始工具参数/结果属性的说明不变。
In `@tool/hostexec/hostexec_safety_test.go`:
- Around line 147-150: Update the Windows command construction in the test to
quote secretFile before passing it to type, while preserving the existing
non-Windows cat command and platform-specific behavior.
In `@tool/permission.go`:
- Around line 192-205: Update ComposeToolResultSanitizers to always return
toolResultSanitizerChain for non-empty filtered sanitizers, removing the
single-sanitizer fast path so nil-argument validation and argument isolation
remain consistent. Add a regression test covering composition with exactly one
sanitizer and verifying the same fail-closed behavior as multi-sanitizer
composition.
In `@tool/safety/audit_open_windows.go`:
- Around line 26-32: Update the Windows audit-file creation flow around
windows.CreateFile to pass an explicit protected security descriptor/DACL
granting access only to the owner, rather than inheriting the directory ACL.
Preserve the existing file-opening behavior and add a Windows-specific ACL test
verifying owner-only access for files created by JSONLSink.
In `@tool/safety/coverage_test.go`:
- Around line 469-499: Strengthen the table-driven RedactValue test by asserting
both got and changed for every container case, including secret bytes, maps,
interface and typed slices, arrays, and non-string-key maps. Verify each result
no longer contains the secret and that changed is true, while preserving the
existing primitive and pointer-specific checks.
---
Nitpick comments:
In `@tool/safety/coverage_test.go`:
- Around line 289-293: Extend the defensive-copy test around Guard.Policy() to
mutate each nested slice in the returned exec ToolProfile, including
AllowedCommands, and verify the active guard policy retains its original values.
Keep the existing map-entry replacement assertion, and ensure the checks detect
shallow-copy leakage across every profile slice exposed by ToolProfile.
- Around line 1091-1097: Update the assertions in the guard.Scan test for
Command "rm -rf /" to stop requiring exactly two findings or relying on
Findings[0]. Locate the relevant custom-rule finding by its rule ID, assert that
it has PermissionActionAsk, and retain the existing external-behavior checks for
the report decision, blocked status, reason, and recommendation.
🪄 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: ab46f42e-df22-47d9-994e-c5cc0e83fb48
📒 Files selected for processing (58)
codeexecutor/codeexecutor.gocodeexecutor/codeexecutor_test.gocodeexecutor/local/local.gocodeexecutor/local/local_test.gocodeexecutor/local/output_limit.gocodeexecutor/local/output_limit_test.gocodeexecutor/local/process_group_unix.gocodeexecutor/local/process_group_windows.gocodeexecutor/local/workspace_runtime.gocodeexecutor/local/workspace_runtime_interactive.gocodeexecutor/workspace.godocs/mkdocs.ymldocs/mkdocs/en/tool-safety-guard.mddocs/mkdocs/zh/tool-safety-guard.mddocs/tool-safety-guard.mddocs/tool-safety-guard.zh-CN.mdexamples/tool_safety_guard/.gitignoreexamples/tool_safety_guard/README.mdexamples/tool_safety_guard/README.zh-CN.mdexamples/tool_safety_guard/acceptance_test.goexamples/tool_safety_guard/main.goexamples/tool_safety_guard/main_test.goexamples/tool_safety_guard/sample/tool_safety_audit.jsonlexamples/tool_safety_guard/sample/tool_safety_report.jsonexamples/tool_safety_guard/tool_safety_policy.yamlgraph/state_graph.gograph/state_graph_test.gointernal/flow/processor/functioncall.gointernal/flow/processor/functioncall_test.gotelemetry/trace/span_attribute_policy.gotelemetry/trace/span_attribute_policy_test.gotool/callbacks.gotool/callbacks_test.gotool/codeexec/codeexec.gotool/codeexec/codeexec_safety_test.gotool/hostexec/hostexec.gotool/hostexec/hostexec_safety_test.gotool/hostexec/manager.gotool/hostexec/procgroup_windows.gotool/hostexec/session.gotool/permission.gotool/permission_test.gotool/safety/adversarial_test.gotool/safety/audit.gotool/safety/audit_open_unix.gotool/safety/audit_open_windows.gotool/safety/audit_test.gotool/safety/coverage_test.gotool/safety/guard.gotool/safety/guard_test.gotool/safety/policy.gotool/safety/policy_test.gotool/safety/redact.gotool/safety/scan.gotool/safety/types.gotool/workspaceexec/workspace_exec.gotool/workspaceexec/workspace_exec_policy_test.gotool/workspaceexec/workspace_exec_safety_test.go
🚧 Files skipped from review as they are similar to previous changes (45)
- examples/tool_safety_guard/.gitignore
- examples/tool_safety_guard/tool_safety_policy.yaml
- codeexecutor/workspace.go
- examples/tool_safety_guard/sample/tool_safety_audit.jsonl
- telemetry/trace/span_attribute_policy.go
- codeexecutor/local/process_group_unix.go
- codeexecutor/local/output_limit.go
- codeexecutor/codeexecutor.go
- telemetry/trace/span_attribute_policy_test.go
- codeexecutor/local/local_test.go
- docs/mkdocs/en/tool-safety-guard.md
- examples/tool_safety_guard/main_test.go
- examples/tool_safety_guard/acceptance_test.go
- tool/callbacks.go
- examples/tool_safety_guard/README.md
- tool/hostexec/procgroup_windows.go
- tool/safety/audit_test.go
- codeexecutor/local/workspace_runtime.go
- codeexecutor/local/process_group_windows.go
- tool/safety/adversarial_test.go
- tool/hostexec/session.go
- tool/workspaceexec/workspace_exec_policy_test.go
- examples/tool_safety_guard/README.zh-CN.md
- tool/safety/types.go
- docs/mkdocs/zh/tool-safety-guard.md
- tool/safety/policy_test.go
- codeexecutor/local/output_limit_test.go
- examples/tool_safety_guard/main.go
- docs/tool-safety-guard.md
- codeexecutor/local/local.go
- examples/tool_safety_guard/sample/tool_safety_report.json
- tool/safety/redact.go
- codeexecutor/local/workspace_runtime_interactive.go
- tool/hostexec/manager.go
- tool/codeexec/codeexec_safety_test.go
- tool/safety/guard_test.go
- tool/workspaceexec/workspace_exec.go
- tool/codeexec/codeexec.go
- graph/state_graph.go
- tool/safety/policy.go
- tool/safety/scan.go
- tool/safety/guard.go
- tool/hostexec/hostexec.go
- internal/flow/processor/functioncall_test.go
- internal/flow/processor/functioncall.go
3f14b4e to
0662812
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tool/safety/audit_open_windows_test.go (1)
15-15: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate the actual ACE rather than matching SDDL text.
Counting only
(A;does not exclude other allow-ACE forms, so an ACL granting another principal could pass. Parse the DACL and assert exactly one allow ACE whose SID equals the current user.中文
请验证实际 ACE，而不是匹配 SDDL 文本。
仅统计
(A;无法排除其他允许型 ACE，因此向其他主体授权的 ACL 仍可能通过测试。请解析 DACL，并断言仅存在一个允许 ACE，且其 SID 等于当前用户。As per path instructions, security assertions must strongly verify the intended behavior.
Also applies to: 48-51
🤖 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/audit_open_windows_test.go` at line 15, Replace the SDDL string-counting assertion in the audit_open_windows test with DACL parsing and inspection of the resulting ACEs. Assert that exactly one allow ACE exists and that its SID matches the current user, rejecting any additional allow ACEs or different principals; update the related assertion block around the referenced lines while preserving the test’s intended ACL setup.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 `@tool/codeexec/codeexec.go`:
- Around line 248-250: Route blocked outcomes from checkSafetyGuard through
finalizeSafetyResult before returning, including both the permission-denied
result and any guard error. Update the early-return handling around
checkSafetyGuard and the corresponding blocked-result path near the later
execution flow, preserving normal validation and execution behavior while
ensuring all policy reasons and guard errors are sanitized consistently.
---
Nitpick comments:
In `@tool/safety/audit_open_windows_test.go`:
- Line 15: Replace the SDDL string-counting assertion in the audit_open_windows
test with DACL parsing and inspection of the resulting ACEs. Assert that exactly
one allow ACE exists and that its SID matches the current user, rejecting any
additional allow ACEs or different principals; update the related assertion
block around the referenced lines while preserving the test’s intended ACL
setup.
🪄 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: 898d55a8-b760-4440-9e7e-264c10e6e012
📒 Files selected for processing (60)
codeexecutor/codeexecutor.gocodeexecutor/codeexecutor_test.gocodeexecutor/local/local.gocodeexecutor/local/local_test.gocodeexecutor/local/output_limit.gocodeexecutor/local/output_limit_test.gocodeexecutor/local/process_group_unix.gocodeexecutor/local/process_group_windows.gocodeexecutor/local/workspace_runtime.gocodeexecutor/local/workspace_runtime_interactive.gocodeexecutor/workspace.godocs/mkdocs.ymldocs/mkdocs/en/tool-safety-guard.mddocs/mkdocs/zh/tool-safety-guard.mddocs/tool-safety-guard.mddocs/tool-safety-guard.zh-CN.mdexamples/tool_safety_guard/.gitignoreexamples/tool_safety_guard/README.mdexamples/tool_safety_guard/README.zh-CN.mdexamples/tool_safety_guard/acceptance_test.goexamples/tool_safety_guard/main.goexamples/tool_safety_guard/main_test.goexamples/tool_safety_guard/sample/tool_safety_audit.jsonlexamples/tool_safety_guard/sample/tool_safety_report.jsonexamples/tool_safety_guard/tool_safety_policy.yamlgo.modgraph/state_graph.gograph/state_graph_test.gointernal/flow/processor/functioncall.gointernal/flow/processor/functioncall_test.gotelemetry/trace/span_attribute_policy.gotelemetry/trace/span_attribute_policy_test.gotool/callbacks.gotool/callbacks_test.gotool/codeexec/codeexec.gotool/codeexec/codeexec_safety_test.gotool/hostexec/hostexec.gotool/hostexec/hostexec_safety_test.gotool/hostexec/manager.gotool/hostexec/procgroup_windows.gotool/hostexec/session.gotool/permission.gotool/permission_test.gotool/safety/adversarial_test.gotool/safety/audit.gotool/safety/audit_open_unix.gotool/safety/audit_open_windows.gotool/safety/audit_open_windows_test.gotool/safety/audit_test.gotool/safety/coverage_test.gotool/safety/guard.gotool/safety/guard_test.gotool/safety/policy.gotool/safety/policy_test.gotool/safety/redact.gotool/safety/scan.gotool/safety/types.gotool/workspaceexec/workspace_exec.gotool/workspaceexec/workspace_exec_policy_test.gotool/workspaceexec/workspace_exec_safety_test.go
🚧 Files skipped from review as they are similar to previous changes (51)
- examples/tool_safety_guard/.gitignore
- examples/tool_safety_guard/sample/tool_safety_audit.jsonl
- telemetry/trace/span_attribute_policy_test.go
- codeexecutor/local/process_group_unix.go
- codeexecutor/codeexecutor.go
- codeexecutor/local/output_limit.go
- tool/safety/audit_test.go
- tool/safety/policy_test.go
- codeexecutor/workspace.go
- examples/tool_safety_guard/main.go
- examples/tool_safety_guard/tool_safety_policy.yaml
- codeexecutor/local/workspace_runtime.go
- telemetry/trace/span_attribute_policy.go
- tool/callbacks_test.go
- codeexecutor/local/local_test.go
- docs/mkdocs/en/tool-safety-guard.md
- codeexecutor/codeexecutor_test.go
- examples/tool_safety_guard/main_test.go
- examples/tool_safety_guard/README.zh-CN.md
- examples/tool_safety_guard/README.md
- tool/hostexec/session.go
- tool/hostexec/procgroup_windows.go
- tool/safety/audit.go
- tool/safety/types.go
- tool/workspaceexec/workspace_exec_safety_test.go
- tool/callbacks.go
- docs/mkdocs/zh/tool-safety-guard.md
- tool/workspaceexec/workspace_exec_policy_test.go
- tool/codeexec/codeexec_safety_test.go
- examples/tool_safety_guard/sample/tool_safety_report.json
- codeexecutor/local/process_group_windows.go
- codeexecutor/local/output_limit_test.go
- tool/safety/adversarial_test.go
- examples/tool_safety_guard/acceptance_test.go
- codeexecutor/local/local.go
- tool/hostexec/manager.go
- tool/safety/policy.go
- tool/safety/redact.go
- tool/permission_test.go
- docs/tool-safety-guard.md
- tool/safety/audit_open_unix.go
- tool/safety/guard_test.go
- tool/permission.go
- tool/hostexec/hostexec.go
- graph/state_graph_test.go
- tool/workspaceexec/workspace_exec.go
- internal/flow/processor/functioncall_test.go
- tool/safety/scan.go
- tool/safety/guard.go
- graph/state_graph.go
- internal/flow/processor/functioncall.go
| if result, blocked, err := t.checkSafetyGuard(ctx, input); blocked { | ||
| return result, err | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
Sanitize permission-denied results and guard errors before returning.
This early return bypasses finalizeSafetyResult, so policy reasons or guard errors can expose secrets. Route blocked outcomes through the same sanitizer used by validation and execution results.
中文
返回前请对权限拒绝结果和 Guard 错误进行脱敏。
这里的提前返回绕过了 finalizeSafetyResult，策略原因或 Guard 错误可能泄露密钥。请让阻止结果也经过与校验及执行结果相同的脱敏流程。
Proposed fix / 建议修复
if result, blocked, err := t.checkSafetyGuard(ctx, input); blocked {
- return result, err
+ return t.finalizeSafetyResult(ctx, args, result, err)
}
func (t *executeCodeTool) finalizeSafetyResult(
ctx context.Context,
args []byte,
- result codeexecutor.CodeExecutionResult,
+ result any,
runErr error,
) (any, error) {Also applies to: 269-292
🤖 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/codeexec/codeexec.go` around lines 248 - 250, Route blocked outcomes
from checkSafetyGuard through finalizeSafetyResult before returning, including
both the permission-denied result and any guard error. Update the early-return
handling around checkSafetyGuard and the corresponding blocked-result path near
the later execution flow, preserving normal validation and execution behavior
while ensuring all policy reasons and guard errors are sanitized consistently.
Rememorio
left a comment
Rememorio
left a comment
There was a problem hiding this comment.
I reviewed the changed lines and left 4 inline comments below. The comments focus on issues worth addressing before merge.
Fresh review found four safety-policy bypasses: guarded Unix host execution still loads login-shell startup files, and framework-only guard configuration misses host execution classification, the declared tty alias, and case-insensitive write-stdin controls. Add parity tests that install the guard only through ToolPermissionPolicy, plus a guarded Unix shell-argument test.
中文
我看过这次变更的相关代码，在下面留下 4 条行内评论。评论聚焦在合并前值得处理的问题。
本次审查发现 4 个安全策略绕过问题：Unix 主机执行启用 Guard 后仍会加载登录 Shell 配置；仅通过框架权限策略配置 Guard 时，还会漏判主机执行、已声明的 tty 参数，以及大小写变体的会话输入控制字段。建议补充仅安装 ToolPermissionPolicy 的端到端测试，并验证 Guard 模式下的 Unix Shell 参数。
| cleanEnv := false | ||
| if guard != nil { | ||
| execEnv = safetyExecutionEnv(t.mgr.baseEnv, in.Env) | ||
| cleanEnv = true |
There was a problem hiding this comment.
P1: Avoid login-shell startup under the guard
Setting cleanEnv here does not make the Unix launch path startup-free: manager.shellSpec still invokes bash or sh with -lc. A login shell can source system and account profile files before the scanned command, allowing unscanned startup code to run and potentially replacing the restricted PATH.
Use non-login -c whenever the guard enables the clean environment, while retaining the historical -lc behavior when no guard is configured. Please also cover the selected shell arguments in a regression test; workspaceexec already makes this distinction for the same profile-injection boundary.
中文
Guard 启用时不要加载登录 Shell 配置
这里设置 cleanEnv 并不能避免 Unix 启动脚本：manager.shellSpec 仍然通过 -lc 启动 bash 或 sh。登录 Shell 会在已扫描命令之前加载系统及用户配置文件，因此可能执行未经 Guard 检查的代码，或者覆盖受限的 PATH。
Guard 启用干净环境时应改用非登录模式 -c，未配置 Guard 时再保留原有的 -lc 行为。同时建议增加回归测试，直接验证最终选择的 Shell 参数；workspaceexec 已经针对同一类配置注入风险做了这一区分。
| return "language" | ||
| case "backend", "executor": | ||
| return "backend" | ||
| case "pty", "use_pty": |
There was a problem hiding this comment.
P1: Normalize the declared tty field before permission checks
The public hostexec and workspaceexec schemas accept tty, but normalization recognizes only pty and use_pty. With an invocation-only guard, {"tty":true} therefore leaves ScanRequest.PTY false and skips the PTY approval check; direct guard configuration hides the issue by re-encoding the value as pty.
Include tty in this alias set and test both built-in tools with the guard installed only at the framework permission layer.
中文
权限检查前应规范化已声明的 tty 字段
hostexec 和 workspaceexec 的公开参数都接受 tty，但这里仅规范化了 pty 和 use_pty。当 Guard 只安装在框架调用层时，{"tty":true} 不会设置 ScanRequest.PTY，从而跳过 PTY 审批；直接配置 Guard 时，适配器会把该值重新编码为 pty，所以测试中不容易暴露问题。
请将 tty 加入别名集合，并针对两个内置工具补充仅在框架权限层安装 Guard 的测试。
| return true | ||
| } | ||
| if req.RawFields == nil { | ||
| return false |
There was a problem hiding this comment.
P1: Honor case-insensitive submit fields in the guard
The Go JSON decoders used by the write-stdin tools match field names case-insensitively, so {"SUBMIT":true} or {"APPEND_NEWLINE":true} sends a newline. This check reads RawFields using exact lowercase keys, so a framework-only guard sees no input and omits the approval decision, while direct configuration catches it after normalized re-encoding.
Canonicalize these raw keys before writeStdinHasInput evaluates them, and add casing regression tests. Otherwise external JSON can interact with an existing session without the policy's expected approval.
中文
Guard 应按不区分大小写识别提交字段
会话输入工具使用的 Go JSON 解码器不区分字段名大小写，因此 {"SUBMIT":true} 或 {"APPEND_NEWLINE":true} 都会实际发送换行。这里却只按完全小写的键读取 RawFields，导致仅在框架层配置的 Guard 判断为“没有输入”并跳过审批；直接配置时由于参数会被重新编码成规范字段，反而能够识别。
建议在 writeStdinHasInput 判断前统一规范化这些原始键，并增加大小写变体的回归测试，否则外部 JSON 可以绕过预期审批与已有会话交互。
|
|
||
| func isHostBackend(backend, toolName string) bool { | ||
| value := strings.ToLower(backend + " " + toolName) | ||
| return strings.Contains(value, "hostexec") || strings.Contains(value, "host_exec") || strings.Contains(value, "host-exec") |
There was a problem hiding this comment.
P1: Classify exec_command as host execution
With a framework-only guard, the original hostexec request reaches the scanner with tool name exec_command and no backend field. This helper does not recognize that name, although effectiveBackend maps it to hostexec, so the default host_execution Ask decision is omitted. Attaching the same guard directly does ask because the adapter injects backend=hostexec.
Classify this check through effectiveBackend or explicitly include exec_command. Add a regression test that installs the guard only as ToolPermissionPolicy and verifies that host execution cannot proceed without approval.
中文
将 exec_command 识别为主机执行
仅在框架权限层安装 Guard 时，hostexec 的原始请求会以工具名 exec_command 且不带 backend 字段进入扫描器。这里没有识别该工具名，尽管 effectiveBackend 已将它映射为 hostexec，因此默认的 host_execution 询问决策会被漏掉。直接把 Guard 配到执行器时，适配器会补上 backend=hostexec，反而能够正确拦截。
建议统一通过 effectiveBackend 判断，或显式加入 exec_command。同时增加仅将 Guard 配置为 ToolPermissionPolicy 的回归测试，确认主机执行未经批准不能继续。
What changed
Why
Tool and CodeExecutor integrations can execute commands, access the network, and expose host data. Sandboxing remains necessary, but it does not provide policy decisions, human-review gates, structured audit records, or secret-safe framework observability. This change adds those controls without changing default behavior: applications opt in by attaching a Guard.
Multiple policies are merged conservatively as
deny > ask > allow. Invalid policy reloads keep the previous snapshot, and audit failures fail closed.Fixes #2002
Testing
tool/safety,tool/codeexec,internal/flow/processor, andgraph.go veton modified packages and example race tests.tool/safetycoverage: 95.1%.Notes for reviewers
Please pay particular attention to permission ordering, callback/Graph redaction barriers, JSONL failure behavior, hostexec PTY cleanup, and the shared stdout/stderr output budget. The committed example scans dangerous strings only; it never executes them.
Windows full-suite failures involving Unix path separators, shell commands, and symlink permissions reproduce on the upstream baseline and are not introduced by this change. The affected new paths are covered by focused Windows tests and Linux race tests.
RELEASE NOTES: Added an opt-in safety guard for tool and code execution with configurable policies, pre-execution review or denial, runtime limits, secret redaction, auditing, and tracing.