test: finops + tool-lookup — RBAC formatting and Zod schema introspection

[anandgupta42]

What does this PR do?

Adds 14 new tests covering two completely untested modules that have real user-facing risk.

1. ToolLookupTool — src/altimate/tools/tool-lookup.ts (4 new tests)

The agent uses tool_lookup to discover other tools' parameter contracts before calling them. The module contains describeZodSchema, inferZodType, unwrap, and getShape — pure functions that introspect Zod's internal _def structure (not a stable public API). Zero tests existed. If a Zod upgrade changes internals or a wrapper type isn't handled by unwrap, the agent silently gets wrong parameter info and makes incorrect tool calls.

New coverage includes:

• Mixed parameter types (string, number, boolean, array, enum) with correct type/required detection
• Default + optional wrapper unwrapping (nested z.string().optional().default(...))
• Empty parameters object handling ("No parameters." path)
• Tool-not-found path with available tools listing

2. FinopsRoleAccess formatting — src/altimate/tools/finops-role-access.ts (10 new tests)

The formatGrants, formatHierarchy, and formatUserRoles functions render RBAC data as tables for data engineers auditing Snowflake permissions. Zero tests existed. Incorrect output could cause engineers to miss security issues.

New coverage includes:

• formatGrants: privilege summary rendering, grant table with standard Snowflake fields, fallback field aliases (grantee_name vs role, object_type vs granted_on, object_name vs name), empty grants, Dispatcher failure error path
• formatHierarchy: recursive two-level tree rendering with children key, granted_roles fallback alias, empty hierarchy
• formatUserRoles: standard field rendering, fallback aliases (user_name, role_name, grantor), empty assignments

Tests use Dispatcher.call spying (established pattern from impact-analysis.test.ts) to supply known RBAC data without requiring a real warehouse connection.

Type of change

• New feature (non-breaking change which adds functionality)

Issue for this PR

N/A — proactive test coverage

How did you verify your code works?

bun test test/altimate/tool-lookup.test.ts         # 4 pass, 19 expect() calls
bun test test/altimate/finops-role-access.test.ts   # 10 pass, 25 expect() calls

Checklist

• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

https://claude.ai/code/session_01PnbJpBRPw8DrqSBDxhrCjg

Summary by CodeRabbit

• Tests
  • Added comprehensive test coverage for finops role-access formatting tools, including validation of role grants, role hierarchies, and user role assignments.
  • Added test coverage for tool lookup functionality, including parameter schema introspection and error handling for unavailable tools.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

_{Tip: disable this comment in your organization's Code Review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Two new Bun test files added to validate finops role-access formatting tools and ToolLookupTool's Zod schema introspection. Tests stub dispatcher calls with deterministic RBAC payloads and assert correct markdown output formatting, fallback alias handling, empty state behavior, and error reporting. Total 397 lines added across both files.

Changes

Cohort / File(s)	Summary
FinOps Role Access Tests packages/opencode/test/altimate/finops-role-access.test.ts	New test suite validating markdown rendering for FinopsRoleGrantsTool, FinopsRoleHierarchyTool, and FinopsUserRolesTool. Tests include privilege summaries, grant/hierarchy tables, fallback alias field support, empty state assertions ("No grants found", "No user role assignments found"), and error handling with dispatcher failure messages.
Tool Lookup Tests packages/opencode/test/altimate/tool-lookup.test.ts	New test suite verifying ToolLookupTool's schema introspection for registered tools. Tests validate parameter metadata extraction from mixed Zod types, "Tool not found" responses with available tools list, empty parameter handling, and nested optional/default wrapper unwrapping behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

contributor

Poem

🐰 Hops of joy for tests so neat,
FinOps roles and lookups sweet,
Mocks and stubs make payloads click,
Assertions verify each trick!
Coverage grows, our code runs quick! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main changes: adding tests for finops role-access RBAC formatting and tool-lookup Zod schema introspection.
Description check	✅ Passed	The description comprehensively explains what changed and why, details the test plan with verification commands, but lacks a formal checklist section matching the template.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/test-finops-tool-lookup-session_01PnbJpBRPw8DrqSBDxhrCjg

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

packages/opencode/test/altimate/finops-role-access.test.ts (1)

18-23: Restore telemetry env value instead of unconditional delete.

Unconditionally deleting the env key can alter parent process config for later tests. Restore the prior value on teardown.

♻️ Proposed fix

+const previousTelemetryDisabled = process.env.ALTIMATE_TELEMETRY_DISABLED
+
 beforeEach(() => {
   process.env.ALTIMATE_TELEMETRY_DISABLED = "true"
 })
 afterAll(() => {
-  delete process.env.ALTIMATE_TELEMETRY_DISABLED
+  if (previousTelemetryDisabled === undefined) {
+    delete process.env.ALTIMATE_TELEMETRY_DISABLED
+  } else {
+    process.env.ALTIMATE_TELEMETRY_DISABLED = previousTelemetryDisabled
+  }
 })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/altimate/finops-role-access.test.ts` around lines 18 -
23, The test currently sets process.env.ALTIMATE_TELEMETRY_DISABLED = "true" in
beforeEach and unconditionally deletes it in afterAll; change this to capture
the prior value before setting (e.g., store in a variable in the test scope),
set the env in beforeEach, and in afterAll restore the previous value (if
undefined, delete the key; otherwise set it back) so
process.env.ALTIMATE_TELEMETRY_DISABLED is returned to its original state;
update the beforeEach/afterAll blocks in finops-role-access.test.ts accordingly.

packages/opencode/test/altimate/tool-lookup.test.ts (1)

16-21: Preserve pre-existing telemetry env state during cleanup.

afterAll always deletes ALTIMATE_TELEMETRY_DISABLED, which can clobber a pre-set value from the parent test process. Restore the previous value instead of always deleting.

♻️ Proposed fix

+const previousTelemetryDisabled = process.env.ALTIMATE_TELEMETRY_DISABLED
+
 beforeEach(() => {
   process.env.ALTIMATE_TELEMETRY_DISABLED = "true"
 })
 afterAll(() => {
-  delete process.env.ALTIMATE_TELEMETRY_DISABLED
+  if (previousTelemetryDisabled === undefined) {
+    delete process.env.ALTIMATE_TELEMETRY_DISABLED
+  } else {
+    process.env.ALTIMATE_TELEMETRY_DISABLED = previousTelemetryDisabled
+  }
 })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/altimate/tool-lookup.test.ts` around lines 16 - 21,
The test currently sets process.env.ALTIMATE_TELEMETRY_DISABLED in beforeEach
and unconditionally deletes it in afterAll, which can clobber a parent process
value; modify the setup to capture the original value (e.g., const prevTelemetry
= process.env.ALTIMATE_TELEMETRY_DISABLED) before changing it in beforeEach and
then in afterAll restore process.env.ALTIMATE_TELEMETRY_DISABLED = prevTelemetry
(or delete it only if prevTelemetry was undefined) so that the original
environment state is preserved; update the beforeEach/afterAll blocks
(referencing beforeEach, afterAll, and ALTIMATE_TELEMETRY_DISABLED) accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/opencode/test/altimate/finops-role-access.test.ts`:
- Around line 38-43: The mockDispatcher implementation incorrectly treats falsy
mocked values as missing; replace the truthiness check in mockDispatcher
(currently `if (responses[method])`) with a property-existence check such as
using Object.prototype.hasOwnProperty.call(responses, method) or the `in`
operator so Dispatcher.call mock returns falsy-but-valid responses instead of
throwing "No mock for ...".

---

Nitpick comments:
In `@packages/opencode/test/altimate/finops-role-access.test.ts`:
- Around line 18-23: The test currently sets
process.env.ALTIMATE_TELEMETRY_DISABLED = "true" in beforeEach and
unconditionally deletes it in afterAll; change this to capture the prior value
before setting (e.g., store in a variable in the test scope), set the env in
beforeEach, and in afterAll restore the previous value (if undefined, delete the
key; otherwise set it back) so process.env.ALTIMATE_TELEMETRY_DISABLED is
returned to its original state; update the beforeEach/afterAll blocks in
finops-role-access.test.ts accordingly.

In `@packages/opencode/test/altimate/tool-lookup.test.ts`:
- Around line 16-21: The test currently sets
process.env.ALTIMATE_TELEMETRY_DISABLED in beforeEach and unconditionally
deletes it in afterAll, which can clobber a parent process value; modify the
setup to capture the original value (e.g., const prevTelemetry =
process.env.ALTIMATE_TELEMETRY_DISABLED) before changing it in beforeEach and
then in afterAll restore process.env.ALTIMATE_TELEMETRY_DISABLED = prevTelemetry
(or delete it only if prevTelemetry was undefined) so that the original
environment state is preserved; update the beforeEach/afterAll blocks
(referencing beforeEach, afterAll, and ALTIMATE_TELEMETRY_DISABLED) accordingly.

🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2c75c5fc-dfa1-4a95-a2fe-8a8c556d9a9c

📥 Commits

Reviewing files that changed from the base of the PR and between 8eb13d4 and c4f5e04.

📒 Files selected for processing (2)

• packages/opencode/test/altimate/finops-role-access.test.ts
• packages/opencode/test/altimate/tool-lookup.test.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use key existence check in mockDispatcher, not truthiness.

if (responses[method]) fails for valid mocked values that are falsy. Check property existence to avoid false “No mock” failures.

🐛 Proposed fix

 function mockDispatcher(responses: Record<string, any>) {
   dispatcherSpy?.mockRestore()
   dispatcherSpy = spyOn(Dispatcher, "call").mockImplementation(async (method: string) => {
-    if (responses[method]) return responses[method]
+    if (Object.prototype.hasOwnProperty.call(responses, method)) return responses[method]
     throw new Error(`No mock for ${method}`)
   })
 }

📝 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

	function mockDispatcher(responses: Record<string, any>) {
	dispatcherSpy?.mockRestore()
	dispatcherSpy = spyOn(Dispatcher, "call").mockImplementation(async (method: string) => {
	if (responses[method]) return responses[method]
	throw new Error(`No mock for ${method}`)
	})
	function mockDispatcher(responses: Record<string, any>) {
	dispatcherSpy?.mockRestore()
	dispatcherSpy = spyOn(Dispatcher, "call").mockImplementation(async (method: string) => {
	if (Object.prototype.hasOwnProperty.call(responses, method)) return responses[method]
	throw new Error(`No mock for ${method}`)
	})
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/altimate/finops-role-access.test.ts` around lines 38 -
43, The mockDispatcher implementation incorrectly treats falsy mocked values as
missing; replace the truthiness check in mockDispatcher (currently `if
(responses[method])`) with a property-existence check such as using
Object.prototype.hasOwnProperty.call(responses, method) or the `in` operator so
Dispatcher.call mock returns falsy-but-valid responses instead of throwing "No
mock for ...".

[anandgupta42]

Consolidated into #403