#2002 Feat/safety scanner - #1
Conversation
4069c32 to
61fea51
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a configurable ChangesTool Safety Guard
Estimated code review effort: 4 (Complex) | ~75 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
tool/safety/redact.go (1)
18-19: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSecret-keyword regexes lack word boundaries, causing over-broad substring matches.
Patterns at lines 18-19 match
secret/token/passwordetc. as bare substrings (e.g.notsecret=value,mytoken:x), which will redact more than intended. This is safety-conservative (over-redaction rather than leaking), so not a correctness blocker, but it can unexpectedly mangle legitimate command/output text reported to users inReport.Command/Report.Evidence. Consider anchoring with\bwhere the preceding character allows it, if false positives become an issue in practice.中文
第18-19行的关键词正则未加词边界,会将 `notsecret=value` 等字符串中的子串一并匹配脱敏。由于结果是"过度脱敏"而非"泄露",不构成阻塞问题,但可能意外破坏回显给用户的命令/证据文本,可视情况加 `\b` 边界收紧匹配。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tool/safety/redact.go` around lines 18 - 19, The secret-matching regexes in redact.go are too broad because they match bare substrings like secret/token/password inside larger words, which can over-redact legitimate text. Update the regexp.MustCompile patterns in the redaction logic to use word boundaries or equivalent anchoring around the keyword alternation so matches only trigger on standalone secret-like keys. Keep the change localized to the regex definitions used by the redaction path that feeds Report.Command and Report.Evidence.tool/safety/policy_test.go (1)
74-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing negative-bound test cases don't cover the zero-value collision noted in policy.go.
Good coverage of negative bounds, but there's no test asserting that an explicit
0formax_timeout_sec/max_output_bytes/etc. is actually preserved throughWithDefaults(as opposed to falling back to the built-in default). GivenValidateexplicitly permits0, consider adding a case to lock in the intended zero-value behavior once the policy.go fix (see companion comment) is applied.中文
现有测试覆盖了负值边界,但缺少验证"显式设置为0"在 `WithDefaults` 后是否被保留的用例。鉴于 `Validate` 明确允许0值,建议在修复 policy.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/safety/policy_test.go` around lines 74 - 116, Add a regression test in policy_test.go to verify that explicit zero values for Policy fields like MaxTimeoutSec, MaxOutputBytes, MaxCommandBytes, and MaxScriptBytes are preserved by Policy.WithDefaults rather than replaced by defaults. Extend TestPolicyValidate_RejectsNegativeBounds or add a companion test that constructs Policy values with 0, calls WithDefaults, and asserts the resulting fields remain 0 before Validate is checked. Use the existing Policy.WithDefaults and Policy.Validate symbols to keep the test aligned with the current behavior.tool/safety/permission.go (1)
112-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded tool-name check reduces generality.
req.ToolName == "execute_code"special-cases a specific tool inside otherwise generic permission logic, contradicting the framework's stated goal of keeping abstractions capability-oriented rather than tied to specific tool identities. Consider deriving this leniency from a capability/metadata flag on the tool call (or a policy-configurable list) instead of a literal string match, so custom or renamed tools with the same failure mode get consistent treatment.As per path instructions, "Whether abstractions remain capability-oriented rather than vendor-oriented, and avoid leaking provider-specific, model-specific, or business-specific assumptions into public layers."中文
`req.ToolName == "execute_code"` 在通用的权限逻辑中对特定工具做了特殊处理，这与框架希望保持能力导向而非绑定具体工具身份的目标相悖。建议从工具调用的能力/元数据标志（或策略可配置列表）派生该宽松处理，而不是硬编码字符串匹配，以便自定义或重命名的同类工具也能获得一致处理。🤖 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/permission.go` around lines 112 - 133, The invalid-arguments branch in permission.go hardcodes a special case for req.ToolName == "execute_code", which makes the generic permission flow tool-name specific. Update the decision logic in the error-handling block around Report construction to derive the softer DecisionAsk/RiskMedium path from a capability or metadata flag on the request (or a configurable policy list) instead of a literal tool name, so equivalent tools are treated consistently.Source: Path instructions
tool/safety/audit_test.go (1)
16-25: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing concurrency-safety test for the "mutex-protected" claim.
No test exercises concurrent
WriteAuditEventcalls to verify the mutex actually prevents interleaved/corrupted JSONL lines under contention, despite that being the documented guarantee ofJSONLAuditWriter.As per path instructions, "Whether concurrency-related changes cover cancellation, shutdown, error propagation, goroutine cleanup, channel ordering, and backpressure."中文
没有测试验证并发调用 `WriteAuditEvent` 时互斥锁能防止 JSONL 行交错/损坏，而这正是 `JSONLAuditWriter` 所声明的保证。🤖 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 16 - 25, The JSONLAuditWriter mutex guarantee is not covered by a concurrent write test. Add a concurrency-safety test in audit_test.go that targets JSONLAuditWriter.WriteAuditEvent by issuing many concurrent calls from multiple goroutines and then asserting the resulting JSONL output stays valid and each line remains complete/non-interleaved. Use the existing failingAuditWriter helper only if needed for setup, and validate the mutex protection under contention rather than just single-call behavior.Source: Path instructions
tool/safety/scanner.go (1)
786-793: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNetwork-command coverage list is narrow.
isNetworkCommandonly recognizescurl, wget, nc, netcat, ssh, scp. Common exfiltration-capable tools liketelnet,ftp,sftp,rsync,ping,curl.exe-wrapped aliases, etc. bypass all network heuristics entirely (they fall throughscanArgvuntouched unless caught by the generic text scanner, which only applies for non-command inputs). Consider making this list policy-configurable or expanding it.中文
`isNetworkCommand` 仅识别 `curl, wget, nc, netcat, ssh, scp`。诸如 `telnet`、`ftp`、`sftp`、`rsync`、`ping` 等具备外传能力的工具完全绕过网络启发式检测。建议将该列表做成策略可配置项或扩充覆盖范围。🤖 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 786 - 793, The isNetworkCommand heuristic in scanner.go is too narrow and misses common network/exfiltration tools, so expand or make configurable the command allowlist/denylist used by scanArgv. Update isNetworkCommand to recognize additional aliases and tools such as telnet, ftp, sftp, rsync, ping, and platform variants like curl.exe, and ensure the logic still routes these commands through the same network scanning path. Keep the change localized to isNetworkCommand and any related scanner policy configuration so future additions are easy to maintain.examples/tool_safety_guard/README.md (1)
66-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid discarding errors in the recommended-usage snippet.
policy, _ := safety.LoadPolicyFile(...)andscanner, _ := safety.NewDefaultScanner(policy)silently drop errors. Readers copying this pattern for a safety-critical scanner could ship with a silently-misconfigured/invalid policy.main.goin the same PR properly checks these errors — the README should match.中文
**推荐用法示例中不应忽略错误。**
policy, _ := safety.LoadPolicyFile(...)与scanner, _ := safety.NewDefaultScanner(policy)直接丢弃了错误。读者若照抄此模式用于安全关键的扫描器，可能在策略无效/加载失败时静默运行。同一 PR 中的main.go已正确处理这些错误，README 应保持一致。♻️ Proposed fix
-policy, _ := safety.LoadPolicyFile("tool_safety_policy.yaml") -scanner, _ := safety.NewDefaultScanner(policy) +policy, err := safety.LoadPolicyFile("tool_safety_policy.yaml") +if err != nil { + // handle error +} +scanner, err := safety.NewDefaultScanner(policy) +if err != nil { + // handle error +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/tool_safety_guard/README.md` around lines 66 - 67, The README’s recommended-usage snippet is silently discarding errors from safety.LoadPolicyFile and safety.NewDefaultScanner, which makes the example unsafe to copy. Update the snippet to match the error handling pattern used in main.go by checking and handling both returned errors explicitly, and keep the example centered on the safety package symbols LoadPolicyFile and NewDefaultScanner so readers see the correct usage.examples/tool_safety_guard/main.go (1)
52-57: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAlign file permissions between audit and report artifacts.
The audit file is created with
os.Create(mode ~0644, world-readable), while the report is written with explicit0o600(Line 80). Both files can contain scan evidence (command text, paths) about tool calls; using the same restrictive permission for both would be more consistent, especially since this is presented as the recommended integration pattern.中文
**审计文件与报告文件的权限应保持一致。**审计文件通过
os.Create创建（默认约 0644，全局可读），而报告文件在第 80 行显式使用0o600。两个文件都可能包含工具调用的扫描证据（命令文本、路径等），作为推荐集成模式，两者使用相同的受限权限会更一致、更安全。🔒️ Proposed fix
-auditFile, err := os.Create(*auditPath) +auditFile, err := os.OpenFile(*auditPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/tool_safety_guard/main.go` around lines 52 - 57, The audit artifact is created with default world-readable permissions while the report uses restrictive permissions, so make them consistent in the audit setup. Update the audit file creation in main.go around the os.Create/auditFile block to use the same 0o600-style restricted mode as the report path, preserving the existing audit writer flow via safety.NewJSONLAuditWriter and the auditFile lifecycle.
🤖 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 66-77: The example in the safety policy setup ignores errors from
LoadPolicyFile and NewDefaultScanner, which can hide missing or invalid policy
configuration. Update the example to handle and surface both errors before
calling runner.Run, and only construct safety.NewPermissionPolicy(scanner) after
policy and scanner are successfully created. Use the LoadPolicyFile and
NewDefaultScanner calls as the key points to add explicit error checks and
failure handling.
In `@tool/safety/permission.go`:
- Around line 134-159: The scan loop in permission.go stops too early when
p.scanner.Scan returns an error, so later ScanRequest entries are never
evaluated. Update the loop in the permission evaluation path to keep scanning
remaining requests after recording the scanner failure, and only call
p.finish(ctx, final) once all requests have been processed. Keep using the
existing final report selection logic with reportRank, and preserve the
synthetic scanner.error failure report for the errored request.
In `@tool/safety/policy.go`:
- Around line 83-95: `Policy.WithDefaults` currently only clears the built-in
deny lists when `DeniedCommands`/`DeniedPaths` are non-nil, so
`DisableDefaultDenies` alone is ignored. Update `WithDefaults` to let
`DisableDefaultDenies` independently force `d.DeniedCommands` and
`d.DeniedPaths` to use the caller’s cleaned values (including empty lists) even
when the inputs are nil, while keeping the existing default-deny behavior when
the flag is false. Use the `Policy` fields and `cleanStringList` logic in
`WithDefaults` to locate and adjust the override flow.
- Around line 82-129: The Policy.WithDefaults method is treating Max* fields as
unset when they are 0, which causes valid zero-valued config to be overwritten
by DefaultPolicy. Update WithDefaults in policy.go to preserve explicit zero
values for MaxTimeoutSec, MaxOutputBytes, MaxCommandBytes, and MaxScriptBytes by
using a presence check rather than a zero check, so Validate and WithDefaults
agree on what “set” means.
In `@tool/safety/scanner.go`:
- Around line 1122-1140: `deleteFlagIsRecursive` can miss recursive deletes when
a combined short flag includes an unknown character alongside `r`. Update the
logic in `deleteFlagIsRecursive` so it detects the presence of `r` even if other
short-flag characters are unrecognized, while still rejecting non-short or
malformed tokens. Keep the existing `deleteTargetIsSystemPath` and
delete-classification flow intact so `command.dangerous_delete` is triggered
whenever recursive deletion is implied.
---
Nitpick comments:
In `@examples/tool_safety_guard/main.go`:
- Around line 52-57: The audit artifact is created with default world-readable
permissions while the report uses restrictive permissions, so make them
consistent in the audit setup. Update the audit file creation in main.go around
the os.Create/auditFile block to use the same 0o600-style restricted mode as the
report path, preserving the existing audit writer flow via
safety.NewJSONLAuditWriter and the auditFile lifecycle.
In `@examples/tool_safety_guard/README.md`:
- Around line 66-67: The README’s recommended-usage snippet is silently
discarding errors from safety.LoadPolicyFile and safety.NewDefaultScanner, which
makes the example unsafe to copy. Update the snippet to match the error handling
pattern used in main.go by checking and handling both returned errors
explicitly, and keep the example centered on the safety package symbols
LoadPolicyFile and NewDefaultScanner so readers see the correct usage.
In `@tool/safety/audit_test.go`:
- Around line 16-25: The JSONLAuditWriter mutex guarantee is not covered by a
concurrent write test. Add a concurrency-safety test in audit_test.go that
targets JSONLAuditWriter.WriteAuditEvent by issuing many concurrent calls from
multiple goroutines and then asserting the resulting JSONL output stays valid
and each line remains complete/non-interleaved. Use the existing
failingAuditWriter helper only if needed for setup, and validate the mutex
protection under contention rather than just single-call behavior.
In `@tool/safety/permission.go`:
- Around line 112-133: The invalid-arguments branch in permission.go hardcodes a
special case for req.ToolName == "execute_code", which makes the generic
permission flow tool-name specific. Update the decision logic in the
error-handling block around Report construction to derive the softer
DecisionAsk/RiskMedium path from a capability or metadata flag on the request
(or a configurable policy list) instead of a literal tool name, so equivalent
tools are treated consistently.
In `@tool/safety/policy_test.go`:
- Around line 74-116: Add a regression test in policy_test.go to verify that
explicit zero values for Policy fields like MaxTimeoutSec, MaxOutputBytes,
MaxCommandBytes, and MaxScriptBytes are preserved by Policy.WithDefaults rather
than replaced by defaults. Extend TestPolicyValidate_RejectsNegativeBounds or
add a companion test that constructs Policy values with 0, calls WithDefaults,
and asserts the resulting fields remain 0 before Validate is checked. Use the
existing Policy.WithDefaults and Policy.Validate symbols to keep the test
aligned with the current behavior.
In `@tool/safety/redact.go`:
- Around line 18-19: The secret-matching regexes in redact.go are too broad
because they match bare substrings like secret/token/password inside larger
words, which can over-redact legitimate text. Update the regexp.MustCompile
patterns in the redaction logic to use word boundaries or equivalent anchoring
around the keyword alternation so matches only trigger on standalone secret-like
keys. Keep the change localized to the regex definitions used by the redaction
path that feeds Report.Command and Report.Evidence.
In `@tool/safety/scanner.go`:
- Around line 786-793: The isNetworkCommand heuristic in scanner.go is too
narrow and misses common network/exfiltration tools, so expand or make
configurable the command allowlist/denylist used by scanArgv. Update
isNetworkCommand to recognize additional aliases and tools such as telnet, ftp,
sftp, rsync, ping, and platform variants like curl.exe, and ensure the logic
still routes these commands through the same network scanning path. Keep the
change localized to isNetworkCommand and any related scanner policy
configuration so future additions are easy to maintain.
🪄 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 Plus
Run ID: d568ed85-b66f-4c23-b852-6637b51ee1dd
📒 Files selected for processing (21)
docs/mkdocs/en/tool.mddocs/mkdocs/zh/tool.mdexamples/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.jsontool/safety/args.gotool/safety/args_test.gotool/safety/audit.gotool/safety/audit_test.gotool/safety/permission.gotool/safety/permission_test.gotool/safety/policy.gotool/safety/policy_test.gotool/safety/redact.gotool/safety/redact_test.gotool/safety/scanner.gotool/safety/scanner_test.gotool/safety/types.gotool/safety/types_test.go
| // WithDefaults fills zero fields with default policy values. | ||
| func (p Policy) WithDefaults() Policy { | ||
| d := DefaultPolicy() | ||
| if p.AllowedCommands != nil { | ||
| d.AllowedCommands = cleanStringList(p.AllowedCommands) | ||
| } | ||
| if p.DeniedCommands != nil && (len(cleanStringList(p.DeniedCommands)) > 0 || p.DisableDefaultDenies) { | ||
| d.DeniedCommands = cleanStringList(p.DeniedCommands) | ||
| } | ||
| if p.DeniedPaths != nil && (len(cleanStringList(p.DeniedPaths)) > 0 || p.DisableDefaultDenies) { | ||
| d.DeniedPaths = cleanStringList(p.DeniedPaths) | ||
| } | ||
| d.DisableDefaultDenies = p.DisableDefaultDenies | ||
| if p.NetworkAllowlist != nil { | ||
| d.NetworkAllowlist = cleanStringList(p.NetworkAllowlist) | ||
| } | ||
| if p.MaxTimeoutSec != 0 { | ||
| d.MaxTimeoutSec = p.MaxTimeoutSec | ||
| } | ||
| if p.MaxOutputBytes != 0 { | ||
| d.MaxOutputBytes = p.MaxOutputBytes | ||
| } | ||
| if p.MaxCommandBytes != 0 { | ||
| d.MaxCommandBytes = p.MaxCommandBytes | ||
| } | ||
| if p.MaxScriptBytes != 0 { | ||
| d.MaxScriptBytes = p.MaxScriptBytes | ||
| } | ||
| if p.EnvAllowlist != nil { | ||
| d.EnvAllowlist = cleanStringList(p.EnvAllowlist) | ||
| } | ||
| if p.DependencyInstallAction != "" { | ||
| d.DependencyInstallAction = p.DependencyInstallAction | ||
| } | ||
| if p.UnparsableShellAction != "" { | ||
| d.UnparsableShellAction = p.UnparsableShellAction | ||
| } | ||
| if p.HostUnparsableAction != "" { | ||
| d.HostUnparsableAction = p.HostUnparsableAction | ||
| } | ||
| if p.SecretAction != "" { | ||
| d.SecretAction = p.SecretAction | ||
| } | ||
| if p.AuditFailureMode != "" { | ||
| d.AuditFailureMode = p.AuditFailureMode | ||
| } | ||
| return d | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Max* zero values are overwritten by defaults
WithDefaults treats 0 as “unset”, but Validate accepts 0 as a valid value. That makes max_timeout_sec: 0, max_output_bytes: 0, etc. impossible to express in config; they fall back to the built-in defaults instead. Use a presence signal if zero should mean “no limit”.
中文
Max* 的 0 值会被默认值覆盖
WithDefaults 把 0 当作“未设置”，但 Validate 接受 0 作为有效值。这样一来，max_timeout_sec: 0、max_output_bytes: 0 等配置无法生效，只会回退到内置默认值。若 0 表示“无限制”，需要用存在性标记来区分。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tool/safety/policy.go` around lines 82 - 129, The Policy.WithDefaults method
is treating Max* fields as unset when they are 0, which causes valid zero-valued
config to be overwritten by DefaultPolicy. Update WithDefaults in policy.go to
preserve explicit zero values for MaxTimeoutSec, MaxOutputBytes,
MaxCommandBytes, and MaxScriptBytes by using a presence check rather than a zero
check, so Validate and WithDefaults agree on what “set” means.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tool/safety/permission_test.go (1)
322-329: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact
PermissionReasontoo
tool/safety/permission.goonly collapses newlines today; a recommendation liketoken=abc123still reachestool.PermissionDecision.Reason, while the JSONL audit path now redacts it. Apply the same redaction here and tightentool/safety/permission_test.goto assert the secret is removed.中文
PermissionReason也需要脱敏
tool/safety/permission.go目前只会折叠换行；像token=abc123这样的内容仍会进入tool.PermissionDecision.Reason，而 JSONL 审计路径已经做了脱敏。这里也应使用同样的脱敏逻辑，并加强tool/safety/permission_test.go，断言密钥已被移除。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tool/safety/permission_test.go` around lines 322 - 329, Update PermissionReason in tool/safety/permission.go to apply the existing redaction logic to recommendation text, in addition to collapsing newlines, before it reaches PermissionDecision.Reason. Tighten the PermissionReason test for the token=abc123 recommendation to assert the secret is absent while preserving the existing non-empty result checks.
🧹 Nitpick comments (3)
tool/safety/scanner_test.go (2)
786-815: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBounded "≤1 false positive" assertion weakens the regression check.
Allowing up to one unspecified non-allow result on a fixed safe-command allowlist means a new false positive can silently appear without failing the test, as long as the count stays at 1. Prefer asserting
0, or if a specific command is a known/accepted false positive, assert on it by name so future regressions are still caught.中文
“允许最多 1 个误报”的断言会削弱该回归测试的有效性：只要计数不超过 1，新出现的误报就不会导致测试失败。建议断言为 `0`，或者如果确实有已知可接受的误报，应针对该具体命令显式断言，以便后续新增的误报仍能被测试捕获。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tool/safety/scanner_test.go` around lines 786 - 815, Strengthen TestDefaultScanner_SafeSampleFalsePositiveRate by requiring falsePositives to equal zero instead of allowing one unspecified non-allow result. Preserve the existing safeCommands scan loop and error checks so any command newly producing a non-Allow decision fails the regression test.
817-843: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWall-clock ≤1s assertions risk CI flakiness.
Per-workload elapsed-time assertions are sensitive to machine load/CI variance. Consider a generous margin,
testing.Short()skip, or relative/ratio-based comparisons instead of an absolute wall-clock bound, per test-stability guidance for this file type.中文
基于绝对耗时（≤1秒）的断言在 CI 环境下容易因机器负载差异而变得不稳定。建议放宽阈值、在 `testing.Short()` 下跳过，或改用相对/比例式比较而非绝对墙钟时间边界，以符合本文件类型对测试稳定性的要求。🤖 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_test.go` around lines 817 - 843, Update TestDefaultScanner_PerformanceSamples to avoid brittle per-workload one-second wall-clock assertions: skip the performance sample in testing.Short() mode and use a substantially more generous timeout or otherwise stable comparison for both scanner.Scan calls while preserving the existing correctness checks.Source: Path instructions
tool/safety/permission_test.go (1)
220-269: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winInvalid later scan discards the prior critical finding's root cause from the audit trail.
This test correctly locks in that
CheckToolPermissionfails closed when any scan iteration returns an invalid decision — good for safety. But per the assertionrequire.NotContains(t, audit.String(), "first.critical_deny"), the specific rule id/evidence for an already-detected critical deny (code block 1) is fully discarded from the audit record, replaced by a genericscanner.invalid_decision. The final action stays correct (Deny), but audit/compliance visibility into why the request was actually dangerous is lost. Consider preserving/merging the highest-severity valid finding's rule id into the invalid-decision report's evidence rather than overwriting it outright.中文
该测试正确锁定了 `CheckToolPermission` 在任一次扫描返回非法决策时都会失败关闭（fail-closed）的安全行为。但根据 `require.NotContains(t, audit.String(), "first.critical_deny")` 断言，已经检测到的严重拒绝（第一个代码块）的具体规则 ID/证据会被完全丢弃，替换为通用的 `scanner.invalid_decision`。最终动作仍然正确（拒绝），但审计/合规视角下丢失了"请求实际因何危险"的根因信息。建议在构造非法决策报告时保留/合并此前已发现的最高严重级别结论，而非直接覆盖。🤖 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/permission_test.go` around lines 220 - 269, Update CheckToolPermission’s invalid-decision handling to retain the highest-severity valid finding encountered before the invalid scan, merging its rule ID and evidence into the final scanner.invalid_decision report while preserving fail-closed denial and invalid-decision details. Adjust the related test expectations so the audit output includes the prior critical finding rather than asserting it is absent.
🤖 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.
Outside diff comments:
In `@tool/safety/permission_test.go`:
- Around line 322-329: Update PermissionReason in tool/safety/permission.go to
apply the existing redaction logic to recommendation text, in addition to
collapsing newlines, before it reaches PermissionDecision.Reason. Tighten the
PermissionReason test for the token=abc123 recommendation to assert the secret
is absent while preserving the existing non-empty result checks.
---
Nitpick comments:
In `@tool/safety/permission_test.go`:
- Around line 220-269: Update CheckToolPermission’s invalid-decision handling to
retain the highest-severity valid finding encountered before the invalid scan,
merging its rule ID and evidence into the final scanner.invalid_decision report
while preserving fail-closed denial and invalid-decision details. Adjust the
related test expectations so the audit output includes the prior critical
finding rather than asserting it is absent.
In `@tool/safety/scanner_test.go`:
- Around line 786-815: Strengthen TestDefaultScanner_SafeSampleFalsePositiveRate
by requiring falsePositives to equal zero instead of allowing one unspecified
non-allow result. Preserve the existing safeCommands scan loop and error checks
so any command newly producing a non-Allow decision fails the regression test.
- Around line 817-843: Update TestDefaultScanner_PerformanceSamples to avoid
brittle per-workload one-second wall-clock assertions: skip the performance
sample in testing.Short() mode and use a substantially more generous timeout or
otherwise stable comparison for both scanner.Scan calls while preserving the
existing correctness checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ca5ad08-7550-433a-992c-eac1f93d6ace
📒 Files selected for processing (7)
tool/safety/audit.gotool/safety/permission.gotool/safety/permission_test.gotool/safety/policy.gotool/safety/policy_test.gotool/safety/scanner.gotool/safety/scanner_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tool/safety/policy.go
- tool/safety/permission.go
- tool/safety/scanner.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tool/safety/permission_test.go (1)
463-505: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood fix for the goroutine-assertion anti-pattern, but the result count isn't verified.
Moving assertions out of the worker goroutines into a channel-collected loop correctly avoids calling
require/t.FailNowfrom a non-test goroutine. However, the final loop never checks that allworkersresults were actually received — if a goroutine exits early without sending (panic, early return, etc.), the range loop just drains fewer items and the test still passes, silently weakening the concurrency verification.As per path instructions, "Whether assertions are strong enough to ensure the intended behavior is actually verified" is a review priority for test files.
♻️ Proposed fix
- for result := range results { - require.NoError(t, result.err) - require.Equal(t, tool.PermissionActionAllow, result.decision.Action) - } + count := 0 + for result := range results { + count++ + require.NoError(t, result.err) + require.Equal(t, tool.PermissionActionAllow, result.decision.Action) + } + require.Equal(t, workers, count)中文
将断言从工作 goroutine 移到通道收集循环中，正确地避免了在非测试 goroutine 中调用
require/t.FailNow。但最终循环没有校验实际收到了workers个结果——如果某个 goroutine 提前退出（panic、提前 return 等)而未发送结果，range 循环只会消费更少的数据，测试仍会通过，从而悄悄削弱了并发验证的强度。根据路径规范，"断言是否足够强以确保预期行为得到真正验证" 是测试文件的审查重点。
建议添加一个计数器并在循环结束后断言其等于
workers。🤖 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/permission_test.go` around lines 463 - 505, Strengthen TestPermissionPolicy_AuditRedactionDoesNotMutateSharedDeniedPaths by counting results consumed from the results channel and asserting the count equals workers after the collection loop. Keep the existing per-result error and decision assertions unchanged.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.
Nitpick comments:
In `@tool/safety/permission_test.go`:
- Around line 463-505: Strengthen
TestPermissionPolicy_AuditRedactionDoesNotMutateSharedDeniedPaths by counting
results consumed from the results channel and asserting the count equals workers
after the collection loop. Keep the existing per-result error and decision
assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6af7585e-cb73-43b5-9341-f9f2a6b865c6
📒 Files selected for processing (4)
tool/safety/permission.gotool/safety/permission_test.gotool/safety/scanner.gotool/safety/scanner_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tool/safety/scanner_test.go
- tool/safety/permission.go
- tool/safety/scanner.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tool/safety/scanner.go (2)
1421-1468: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
deleteFlagIsRecursivestill drops mixed short flags like-rq
The helper rejects the whole token as soon as it sees an unlisted character, so any combined short flag that includesrbut also another flag (for example-rq) is still classified as non-recursive and can fall through tocommand.delete. Parse the short-flag bundle by checking forrbefore rejecting unknown letters, or otherwise handle mixed tokens explicitly.中文
deleteFlagIsRecursive仍会误判混合短选项，例如-rq
当前实现一旦遇到未白名单的字符就直接返回false，因此只要短选项里包含r但还带有其他旗标（例如-rq），就可能被判成非递归并落到command.delete。需要在拒绝未知字符前先识别r，或显式处理混合短选项。🤖 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 1421 - 1468, Update deleteFlagIsRecursive to recognize the recursive r flag in mixed short-option bundles such as -rq before rejecting unknown characters, while preserving existing handling for long options and non-recursive arguments.
217-255: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
EnvAllowlistshould be able to exemptenv.process_control(tool/safety/scanner.go:217-255)
scanEnvalways emitsenv.process_controlforPATH/LD_*/etc., even when the key is explicitly present inPolicy.EnvAllowlist. That makes the allowlist ineffective for documented configs likeexamples/tool_safety_guard/tool_safety_policy.yamland blocks legitimate environment overrides. Gate the process-control finding ons.envAllowed(key), or update the policy/docs if this deny is meant to be unconditional.中文
`scanEnv` 对 `PATH`/`LD_*` 等变量会无条件生成 `env.process_control`，即使这些键已经显式写入 `Policy.EnvAllowlist` 也一样。这样会让允许列表对已文档化的配置失效，并阻塞合法的环境覆盖。建议让 `env.process_control` 也检查 `s.envAllowed(key)`，或者如果这里确实要强制拒绝，就同步更新 policy/docs。🤖 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 217 - 255, Update DefaultScanner.scanEnv so the env.process_control finding is emitted only when the key is not permitted by s.envAllowed(key), allowing explicitly allowlisted process-control variables while preserving the existing finding behavior for non-allowlisted keys and other environment checks.
🤖 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/redact.go`:
- Line 47: Update credentialURLPattern used by redactString and containsSecret
to match any RFC-style URL scheme rather than only http, https, and ftp,
including DSNs such as postgres:// and mongodb://. Keep the existing URL parsing
and redaction behavior unchanged after matching, and treat matched URLs as
untrusted external input.
---
Outside diff comments:
In `@tool/safety/scanner.go`:
- Around line 1421-1468: Update deleteFlagIsRecursive to recognize the recursive
r flag in mixed short-option bundles such as -rq before rejecting unknown
characters, while preserving existing handling for long options and
non-recursive arguments.
- Around line 217-255: Update DefaultScanner.scanEnv so the env.process_control
finding is emitted only when the key is not permitted by s.envAllowed(key),
allowing explicitly allowlisted process-control variables while preserving the
existing finding behavior for non-allowlisted keys and other environment checks.
🪄 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 Plus
Run ID: f606f92b-f8a4-43fe-9a46-29e280339453
📒 Files selected for processing (9)
tool/safety/args.gotool/safety/args_test.gotool/safety/permission_test.gotool/safety/policy.gotool/safety/policy_test.gotool/safety/redact.gotool/safety/redact_test.gotool/safety/scanner.gotool/safety/scanner_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- tool/safety/args_test.go
- tool/safety/redact_test.go
- tool/safety/policy_test.go
- tool/safety/policy.go
- tool/safety/args.go
- tool/safety/permission_test.go
1 participant
for review