test(toolpolicy): cover deny audit records - #84
Open
XnLemon wants to merge 1 commit into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughEnglish
中文
WalkthroughAdds a test covering denial of a non-whitelisted tool, audit record identity fields, redaction versioning, and exclusion of sensitive argument content from the audit detail reference. ChangesTool policy audit behavior
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/toolpolicy/policy_test.go`:
- Around line 79-114: Strengthen the audit record assertions in the
CheckToolPermission test by validating record.UserIDHash against the expected
platform.UserIDHash("tenant", "wecom", "external-user") value. Keep the existing
context and other audit field checks unchanged, ensuring the hashed user
identity is verified without accepting an empty or raw external user ID.
🪄 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: 3ee1b1bf-71e8-4604-8725-09640cbad031
📒 Files selected for processing (1)
platform/toolpolicy/policy_test.go
Comment on lines
+79
to
+114
| decision, err := p.CheckToolPermission( | ||
| ContextWithAuditContext(context.Background(), AuditContext{ | ||
| Channel: "wecom", | ||
| BindingID: "binding", | ||
| SessionID: "session", | ||
| InternalUserID: "internal-user", | ||
| UserIDHash: platform.UserIDHash("tenant", "wecom", "external-user"), | ||
| RequestID: "request-1", | ||
| AgentName: "agent", | ||
| }), | ||
| request("shell", tool.ToolMetadata{}, []byte(`{"command":"rm -rf /tmp/demo","Authorization":"Bearer raw-token"}`)), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("CheckToolPermission: %v", err) | ||
| } | ||
| if decision.Action != tool.PermissionActionDeny { | ||
| t.Fatalf("expected deny, got %+v", decision) | ||
| } | ||
|
|
||
| records := audit.Records() | ||
| if len(records) != 1 { | ||
| t.Fatalf("expected one audit record, got %+v", records) | ||
| } | ||
| record := records[0] | ||
| if record.TenantID != "tenant" || | ||
| record.AppID != "app" || | ||
| record.Channel != "wecom" || | ||
| record.BindingID != "binding" || | ||
| record.SessionID != "session" || | ||
| record.InternalUserID != "internal-user" || | ||
| record.RequestID != "request-1" || | ||
| record.AgentName != "agent" || | ||
| record.ToolName != "shell" || | ||
| record.Decision != string(tool.PermissionActionDeny) { | ||
| t.Fatalf("unexpected audit record: %+v", record) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assertions don't verify UserIDHash, despite it being set in the test's AuditContext.
The test populates UserIDHash via platform.UserIDHash(...) (Line 85), and applyAuditContext is known to copy it onto the audit record, but the validation block (Lines 103-114) never checks record.UserIDHash. This leaves a real regression path (e.g. hash function change, empty-string leak, or accidental exposure of the raw external user ID) uncaught. The PR objective explicitly calls out "user" as part of the expected audit context.
As per path instructions, "Whether assertions are strong enough to ensure the intended behavior is actually verified" should be checked for **/*_test.go files.
🐛 Proposed fix
if record.TenantID != "tenant" ||
record.AppID != "app" ||
record.Channel != "wecom" ||
record.BindingID != "binding" ||
record.SessionID != "session" ||
record.InternalUserID != "internal-user" ||
+ record.UserIDHash != platform.UserIDHash("tenant", "wecom", "external-user") ||
record.RequestID != "request-1" ||
record.AgentName != "agent" ||
record.ToolName != "shell" ||
record.Decision != string(tool.PermissionActionDeny) {
t.Fatalf("unexpected audit record: %+v", record)
}中文
断言未验证 UserIDHash，尽管测试的 AuditContext 中已设置该值。
测试在第 85 行通过 platform.UserIDHash(...) 设置了 UserIDHash，且已知 applyAuditContext 会将其复制到审计记录，但第 103-114 行的校验块从未检查 record.UserIDHash。这会导致真实的回归路径（例如哈希函数变更、空字符串泄漏，或意外暴露原始外部用户 ID）无法被捕获。PR 目标中明确提到"user"是预期审计上下文的一部分。
根据路径指令，**/*_test.go 文件应检查"断言是否足够严格以确保预期行为得到实际验证"。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| decision, err := p.CheckToolPermission( | |
| ContextWithAuditContext(context.Background(), AuditContext{ | |
| Channel: "wecom", | |
| BindingID: "binding", | |
| SessionID: "session", | |
| InternalUserID: "internal-user", | |
| UserIDHash: platform.UserIDHash("tenant", "wecom", "external-user"), | |
| RequestID: "request-1", | |
| AgentName: "agent", | |
| }), | |
| request("shell", tool.ToolMetadata{}, []byte(`{"command":"rm -rf /tmp/demo","Authorization":"Bearer raw-token"}`)), | |
| ) | |
| if err != nil { | |
| t.Fatalf("CheckToolPermission: %v", err) | |
| } | |
| if decision.Action != tool.PermissionActionDeny { | |
| t.Fatalf("expected deny, got %+v", decision) | |
| } | |
| records := audit.Records() | |
| if len(records) != 1 { | |
| t.Fatalf("expected one audit record, got %+v", records) | |
| } | |
| record := records[0] | |
| if record.TenantID != "tenant" || | |
| record.AppID != "app" || | |
| record.Channel != "wecom" || | |
| record.BindingID != "binding" || | |
| record.SessionID != "session" || | |
| record.InternalUserID != "internal-user" || | |
| record.RequestID != "request-1" || | |
| record.AgentName != "agent" || | |
| record.ToolName != "shell" || | |
| record.Decision != string(tool.PermissionActionDeny) { | |
| t.Fatalf("unexpected audit record: %+v", record) | |
| } | |
| decision, err := p.CheckToolPermission( | |
| ContextWithAuditContext(context.Background(), AuditContext{ | |
| Channel: "wecom", | |
| BindingID: "binding", | |
| SessionID: "session", | |
| InternalUserID: "internal-user", | |
| UserIDHash: platform.UserIDHash("tenant", "wecom", "external-user"), | |
| RequestID: "request-1", | |
| AgentName: "agent", | |
| }), | |
| request("shell", tool.ToolMetadata{}, []byte(`{"command":"rm -rf /tmp/demo","Authorization":"Bearer raw-token"}`)), | |
| ) | |
| if err != nil { | |
| t.Fatalf("CheckToolPermission: %v", err) | |
| } | |
| if decision.Action != tool.PermissionActionDeny { | |
| t.Fatalf("expected deny, got %+v", decision) | |
| } | |
| records := audit.Records() | |
| if len(records) != 1 { | |
| t.Fatalf("expected one audit record, got %+v", records) | |
| } | |
| record := records[0] | |
| if record.TenantID != "tenant" || | |
| record.AppID != "app" || | |
| record.Channel != "wecom" || | |
| record.BindingID != "binding" || | |
| record.SessionID != "session" || | |
| record.InternalUserID != "internal-user" || | |
| record.UserIDHash != platform.UserIDHash("tenant", "wecom", "external-user") || | |
| record.RequestID != "request-1" || | |
| record.AgentName != "agent" || | |
| record.ToolName != "shell" || | |
| record.Decision != string(tool.PermissionActionDeny) { | |
| t.Fatalf("unexpected audit record: %+v", record) | |
| } |
🤖 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 `@platform/toolpolicy/policy_test.go` around lines 79 - 114, Strengthen the
audit record assertions in the CheckToolPermission test by validating
record.UserIDHash against the expected platform.UserIDHash("tenant", "wecom",
"external-user") value. Keep the existing context and other audit field checks
unchanged, ensuring the hashed user identity is verified without accepting an
empty or raw external user ID.
Source: Path instructions
XnLemon
force-pushed
the
codex/phase2-redactor-spaced-secrets
branch
from
July 11, 2026 13:38
170433e to
fb85850
Compare
XnLemon
force-pushed
the
codex/phase2-tool-deny-audit
branch
from
July 11, 2026 13:38
ffe5fe3 to
75002b5
Compare
XnLemon
changed the base branch from
codex/phase2-redactor-spaced-secrets
to
main
July 13, 2026 02:23
XnLemon
force-pushed
the
codex/phase2-tool-deny-audit
branch
from
July 13, 2026 02:33
75002b5 to
a5f16ec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
Tighten Phase2 acceptance coverage for unauthorized tool denial audit logs.
Changes
CheckToolPermission.RedactedDetailRefcontains only safe digest/bytes/decision details and does not leak raw tool args, Authorization headers, tokens, or paths.Validation
go test ./platform/toolpolicy ./platformgo vet ./platform/toolpolicy ./platformgit diff --check0 High / 0 Medium / 0 LowRisks / Follow-up