Skip to content

test: add unit coverage for permissions module - #49

Open
Ayush7614 wants to merge 1 commit into
KlaatAI:mainfrom
Ayush7614:test/permissions-unit-tests
Open

test: add unit coverage for permissions module#49
Ayush7614 wants to merge 1 commit into
KlaatAI:mainfrom
Ayush7614:test/permissions-unit-tests

Conversation

@Ayush7614

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • bun run typecheck
  • bun test
  • bun run build

Cover summarizeTool summaries, allow/deny glob matching via
checkPermission, and truncateToolResult edge cases.
@github-actions

Copy link
Copy Markdown
Contributor

🤖 KlaatAI Review Bot (powered by Klaatu, advisory only — a maintainer makes the real call)

Issue match

Fully addresses #5. The diff adds src/permissions/index.test.ts covering summarizeTool for all requested tools (read_file, write_file, edit_file, run_command, apply_patch) and the requested glob-pattern matching. It also exceeds the scope by testing truncateToolResult.

Test coverage

Tests assert exact summary strings and permission outcomes. However, while allowed_commands has both match and non-match cases, denied_commands only tests a match (rm -rf /). A test confirming a non-matching command (e.g., rm file.txt against denied_commands: ["rm -rf /"]) yields "ask" or "allow" is missing.

Correctness concerns

No obvious risks. The run_command truncation test correctly bounds the length and checks for the ellipsis. The tc helper hardcodes id: "call_test", which is fine for these pure logic tests but would require updating if future tests rely on unique IDs.

Verdict

Ready to merge as-is, though adding a non-match assertion for denied_commands would cleanly satisfy the issue's pattern-matching acceptance criteria.

This is an automated review to help triage faster, not a gate. Nothing here blocks merging.

@Prateekgaur

Copy link
Copy Markdown
Collaborator

One gap worth closing before merge: we test that a command matching the deny list gets blocked, but we don't test that a command not on the deny list is left alone. Right now nothing proves the deny rule is precise rather than accidentally blocking more than it should.

Could we add one more case — something like a harmless command (e.g. rm file.txt) checked against a deny list that only contains rm -rf / — and confirm it's not denied? That closes the loop on the pattern-matching behaviour and matches the acceptance criteria in the original issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit tests for src/permissions/index.ts

2 participants