Skip to content

test(toolpolicy): cover deny audit records #84

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
XnLemon wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test(toolpolicy): cover deny audit records #84

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions platform/toolpolicy/policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,66 @@
}
}

func TestPolicyDeniesToolOutsideWhitelistWithAudit(t *testing.T) {
audit := platform.NewInMemoryAuditSink()
p := newPolicy(t, platform.ToolPolicy{
ToolWhitelist: []string{"knowledge_search"},
}, WithAuditSink(audit))

decision, err := p.CheckToolPermission(
ContextWithAuditContext(context.Background(), AuditContext{

Check failure on line 69 in platform/toolpolicy/policy_test.go

View workflow job for this annotation

GitHub Actions / lint 

undefined: AuditContext (typecheck)

Check failure on line 69 in platform/toolpolicy/policy_test.go

View workflow job for this annotation

GitHub Actions / lint 

undefined: ContextWithAuditContext

Check failure on line 69 in platform/toolpolicy/policy_test.go

View workflow job for this annotation

GitHub Actions / go test (go.mod) 

undefined: AuditContext

Check failure on line 69 in platform/toolpolicy/policy_test.go

View workflow job for this annotation

GitHub Actions / go test (go.mod) 

undefined: ContextWithAuditContext
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)
}
Comment on lines +68 to +103

@coderabbitai coderabbitai Bot Jul 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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

if record.AuditID == "" || record.RedactionVersion != "platform-toolpolicy-v1" {
t.Fatalf("expected audit id and redaction version, got %+v", record)
}
if !strings.Contains(record.DecisionReason, "whitelist") {
t.Fatalf("expected whitelist decision reason, got %q", record.DecisionReason)
}
if !strings.Contains(record.RedactedDetailRef, "decision:deny") ||
!strings.Contains(record.RedactedDetailRef, "args:sha256:") ||
!strings.Contains(record.RedactedDetailRef, "args_bytes:") {
t.Fatalf("expected safe denial detail summary, got %q", record.RedactedDetailRef)
}
for _, leaked := range []string{"rm -rf", "raw-token", "Authorization", "/tmp/demo"} {
if strings.Contains(record.RedactedDetailRef, leaked) {
t.Fatalf("audit leaked raw argument content %q: %q", leaked, record.RedactedDetailRef)
}
}
}

func TestPolicyAsksForHighRiskTool(t *testing.T) {
p := newPolicy(t, platform.ToolPolicy{
DangerousToolAction: platform.DangerousToolActionAsk,
Expand Down
Loading