test: lazy utility and credential-store — error retry, reset, sensitive field coverage

[anandgupta42]

What does this PR do?

1. lazy() — src/util/lazy.ts (3 new tests)

The lazy() utility powers critical initialization paths including Shell.preferred and Shell.acceptable (which determine which shell is used for all tool execution). The existing 3 tests only covered basic memoization and type support — two important behaviors were completely untested:

• Error non-caching: When the factory function throws (e.g., a transient which failure during shell detection), the error must NOT be cached. The next call should retry the factory. Without this, a single transient failure would permanently break shell detection for the entire session.
• reset() method: Used in Shell tests' beforeEach/afterEach and by the file watcher, but never directly tested. Verifies that reset() clears both the cached value and the loaded flag, causing the factory to be re-invoked on the next call.

Specific scenarios covered:

• reset() clears cached value and re-invokes factory
• Factory error is not cached — retries on next call
• reset() after error allows fresh initialization

2. isSensitiveField — src/altimate/native/connections/credential-store.ts (1 new test)

The SENSITIVE_FIELDS set in credential-store.ts contains 20 field names that should be stripped from config files and stored in the OS keychain. The existing unit test for isSensitiveField covered 14 of 20 fields. This PR adds explicit assertions for the remaining 6 fields:

• credentials_json — BigQuery service account credentials
• keyfile_json — BigQuery keyfile content
• ssl_key, ssl_cert, ssl_ca — TLS/SSL connection credentials
• ssh_password — SSH tunnel authentication

These fields are already correctly handled by the implementation, but without explicit test coverage a future refactor could accidentally remove them from the set without any test failure.

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/util/lazy.test.ts           # 6 pass (3 existing + 3 new)
bun test test/altimate/connections.test.ts # 69 pass (68 existing + 1 new)

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_01WoqeutgfwXNcktweCKoLwd

Summary by CodeRabbit

Tests

• Enhanced test coverage for credential field security validation to ensure sensitive credential fields are properly protected
• Expanded test suite for lazy loading utilities, including reset functionality and error handling scenarios

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

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

[coderabbitai]

📝 Walkthrough

Walkthrough

Two test files were expanded with additional unit test cases: credential field sensitivity detection now includes credentials_json, keyfile_json, ssl_key, ssl_cert, ssl_ca, and ssh_password assertions; lazy-loading utilities now test reset() behavior and error handling scenarios across multiple failure and recovery paths.

Changes

Cohort / File(s)	Summary
Test Coverage Expansions packages/opencode/test/altimate/connections.test.ts, packages/opencode/test/util/lazy.test.ts	Added test assertions for sensitive credential field detection and lazy-loading reset/error handling behavior including factory re-invocation, error non-caching, and recovery scenarios.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels

contributor

Poem

🐰 Test cases multiply, hooray, hooray!
Credentials now hide in a safer way,
Lazy loaders reset with grace,
Errors caught in their proper place,
Coverage grows stronger each test day! 🧪

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and specifically describes the main changes: adding test coverage for lazy utility error retry and reset, plus credential-store sensitive field coverage.
Description check	✅ Passed	The description provides comprehensive detail on what changed and why, includes test verification results, and checks all required template sections (Summary, Test Plan via checklist, and Checklist).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ 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 test/hourly-20260328-0414

---

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]

🧹 Nitpick comments (3)

packages/opencode/test/util/lazy.test.ts (1)

37-78: Consider removing PR-scoped marker comments before merge.

// altimate_change start/end looks like temporary review scaffolding and can be dropped to keep test files clean.

🤖 Prompt for AI Agents

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

In `@packages/opencode/test/util/lazy.test.ts` around lines 37 - 78, Remove the
temporary PR-scoped marker comments "// altimate_change start" and "//
altimate_change end" that wrap the added tests; edit the test file to delete
those two comment lines so the three tests ("reset() clears cached value and
re-invokes factory", "factory error is not cached — retries on next call", and
"reset() after error allows fresh initialization") remain intact and unchanged
(references: the lazy() usage and getValue.reset() in the tests) with no
scaffold comments left.

packages/opencode/test/altimate/connections.test.ts (2)

839-845: Empty catch block silently swallows connection errors.

While acceptable for retry scenarios, logging a debug message would aid troubleshooting when the native binding fails unexpectedly.

♻️ Consider adding minimal logging

       } catch {
+        // Uncomment for debugging native binding issues:
+        // console.debug(`DuckDB connection attempt ${attempt} failed`)
         if (attempt < maxAttempts) {
           // Brief delay before retry to let concurrent native-binding loads settle
           await new Promise((r) => setTimeout(r, 100 * attempt))
         }
         // Native binding unavailable — tests will skip via duckdbReady guard
       }

🤖 Prompt for AI Agents

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

In `@packages/opencode/test/altimate/connections.test.ts` around lines 839 - 845,
Change the empty catch to capture the error (e.g., catch (err)) and emit a
minimal debug log with the error and current retry state so connection failures
aren’t silently swallowed; include attempt and maxAttempts in the message and
use an existing debug/logging mechanism (for example console.debug or your test
logger) so the duckdbReady guard behavior can be traced when native binding load
fails.

---

857-864: Use test.skipIf instead of early returns for clearer test skipping.

The if (!duckdbReady) return pattern silently passes skipped tests, which can be misleading in CI reporting. Bun's test.skipIf supports runtime evaluation of conditions, so you can refactor the tests to use it. Since duckdbReady is set in beforeAll, the skip condition will evaluate to the correct value before test execution.

♻️ Suggested refactor

-  test("execute SELECT 1", async () => {
-    if (!duckdbReady) return
+  test.skipIf(!duckdbReady)("execute SELECT 1", async () => {
     const result = await connector.execute("SELECT 1 AS num")
     expect(result.columns).toEqual(["num"])
     expect(result.rows).toEqual([[1]])
     expect(result.row_count).toBe(1)
     expect(result.truncated).toBe(false)
   })

Apply the same pattern to all other tests in this block.

🤖 Prompt for AI Agents

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

In `@packages/opencode/test/altimate/connections.test.ts` around lines 857 - 864,
Replace the early-return skip pattern in these tests with Bun's runtime-aware
test.skipIf: instead of beginning the test body with "if (!duckdbReady) return",
use test.skipIf(!duckdbReady, "DuckDB not ready") (or the equivalent skipIf
wrapper) so the test is reported as skipped rather than silently passing; update
the SELECT test that calls connector.execute("SELECT 1 AS num") (and apply the
same change to the other tests in this block that reference duckdbReady and
connector) to use test.skipIf(!duckdbReady, ...) so the skip is evaluated
correctly after beforeAll sets duckdbReady.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@packages/opencode/test/altimate/connections.test.ts`:
- Around line 839-845: Change the empty catch to capture the error (e.g., catch
(err)) and emit a minimal debug log with the error and current retry state so
connection failures aren’t silently swallowed; include attempt and maxAttempts
in the message and use an existing debug/logging mechanism (for example
console.debug or your test logger) so the duckdbReady guard behavior can be
traced when native binding load fails.
- Around line 857-864: Replace the early-return skip pattern in these tests with
Bun's runtime-aware test.skipIf: instead of beginning the test body with "if
(!duckdbReady) return", use test.skipIf(!duckdbReady, "DuckDB not ready") (or
the equivalent skipIf wrapper) so the test is reported as skipped rather than
silently passing; update the SELECT test that calls connector.execute("SELECT 1
AS num") (and apply the same change to the other tests in this block that
reference duckdbReady and connector) to use test.skipIf(!duckdbReady, ...) so
the skip is evaluated correctly after beforeAll sets duckdbReady.

In `@packages/opencode/test/util/lazy.test.ts`:
- Around line 37-78: Remove the temporary PR-scoped marker comments "//
altimate_change start" and "// altimate_change end" that wrap the added tests;
edit the test file to delete those two comment lines so the three tests
("reset() clears cached value and re-invokes factory", "factory error is not
cached — retries on next call", and "reset() after error allows fresh
initialization") remain intact and unchanged (references: the lazy() usage and
getValue.reset() in the tests) with no scaffold comments left.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b0569021-4a2f-4961-8141-75855fb9d772

📥 Commits

Reviewing files that changed from the base of the PR and between 9ba2114 and 0ac0b71.

📒 Files selected for processing (2)

• packages/opencode/test/altimate/connections.test.ts
• packages/opencode/test/util/lazy.test.ts

[anandgupta42]

Consolidated into #545