Skip to content

test: connections — SSH tunnel + MongoDB auth detection#512

Closed
anandgupta42 wants to merge 1 commit intomainfrom
test/hourly-20260327-1522
Closed

test: connections — SSH tunnel + MongoDB auth detection#512
anandgupta42 wants to merge 1 commit intomainfrom
test/hourly-20260327-1522

Conversation

@anandgupta42
Copy link
Copy Markdown
Contributor

@anandgupta42 anandgupta42 commented Mar 27, 2026

What does this PR do?

Adds 11 new unit tests covering two untested areas in the warehouse connection layer. Both modules sit in the critical path for database connectivity — bugs here cause silent connection failures or incorrect telemetry.

1. extractSshConfigsrc/altimate/native/connections/ssh-tunnel.ts (7 new tests)

This pure function extracts SSH tunnel configuration from a ConnectionConfig and validates incompatible options. It had zero test coverage despite being called on every warehouse connection attempt when SSH fields are present. New coverage includes:

  • Returns null when no ssh_host is present (early-return guard)
  • Extracts all SSH fields correctly when fully specified
  • Applies correct defaults: ssh_port=22, ssh_user="root", host="127.0.0.1", port=5432
  • Throws a clear error when connection_string is combined with SSH tunnel (user-facing validation)
  • Supports private key authentication (ssh_private_key populated, ssh_password undefined)
  • closeTunnel is idempotent on non-existent tunnels and doesn't corrupt state
  • getActiveTunnel returns undefined for non-existent tunnels

2. detectAuthMethod MongoDB paths — src/altimate/native/connections/registry.ts (4 new tests)

The MongoDB driver was added in #482 (commit abcaa1d), introducing a new type-specific fallback branch in detectAuthMethod. This branch had zero test coverage. New coverage includes:

  • { type: "mongodb" } (no password) → falls through to MongoDB-specific branch returning "connection_string"
  • { type: "mongo" } alias → same behavior via the "mongo" alias
  • { type: "mongodb", password: "..." } → caught by generic password check (documents precedence)
  • { type: "mongodb", connection_string: "..." } → caught by generic connection_string check (documents precedence)

Type of change

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

Issue for this PR

N/A — proactive test coverage for recently added MongoDB driver support and untested SSH tunnel extraction logic.

How did you verify your code works?

bun test test/altimate/ssh-tunnel-and-registry.test.ts   # 11 pass, 17 expect() calls

Marker check: bun script/upstream/analyze.ts --markers --base main --strict → pass (no upstream files modified).

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_01Wr8GYaTQDKpHV1UQDftUJr

Summary by CodeRabbit

  • Tests
    • Added comprehensive test coverage for SSH tunnel configuration extraction, including handling of optional credentials and default values.
    • Added tests for tunnel state operations to ensure safe handling of non-existent tunnels.
    • Added tests for registry authentication method detection and precedence.

…hMethod

Cover two untested areas in the warehouse connection layer:
1. extractSshConfig: zero prior coverage for the function that extracts SSH
   tunnel config and validates connection_string incompatibility.
2. detectAuthMethod MongoDB paths: newly added in #482 (MongoDB driver support)
   with no test coverage for the type-specific fallback branch.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

https://claude.ai/code/session_01Wr8GYaTQDKpHV1UQDftUJr
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

📝 Walkthrough

Walkthrough

A new Bun test file has been added to validate SSH tunnel configuration extraction, tunnel lifecycle helpers, and registry authentication method precedence. The tests cover default value application, parameter validation, and MongoDB-specific authentication behavior.

Changes

Cohort / File(s) Summary
SSH Tunnel and Registry Tests
packages/opencode/test/altimate/ssh-tunnel-and-registry.test.ts
New test suite covering extractSshConfig with null handling and default value application, closeTunnel/getActiveTunnel safety checks, and detectAuthMethod precedence validation for MongoDB connections.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A tunneling test hopped into sight,
With SSH configs shining bright!
MongoDB dances, defaults align,
Auth methods precedent and fine!
Our warren's security—now tested right! 🔐

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and clearly describes the main change: adding test coverage for SSH tunnel and MongoDB authentication detection logic.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering what changed and why with detailed context. However, it does not strictly follow the required template structure (missing explicit 'Summary', 'Test Plan', and 'Checklist' sections as formatted in the template).
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-20260327-1522

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

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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/ssh-tunnel-and-registry.test.ts`:
- Around line 4-5: The test setup currently unconditionally sets and then
deletes ALTIMATE_TELEMETRY_DISABLED in beforeAll/afterAll; modify beforeAll to
capture the original value (e.g. const _orig =
process.env.ALTIMATE_TELEMETRY_DISABLED), then set
process.env.ALTIMATE_TELEMETRY_DISABLED = "true", and modify afterAll to restore
the original: if (_orig === undefined) delete
process.env.ALTIMATE_TELEMETRY_DISABLED else
process.env.ALTIMATE_TELEMETRY_DISABLED = _orig; keep the references to the same
beforeAll and afterAll functions and the ALTIMATE_TELEMETRY_DISABLED env var so
the test no longer leaks global state.
- Around line 67-70: The test currently embeds a PEM-formatted private key
literal in the assertions (the string compared to result!.ssh_private_key),
which triggers secret scanners; replace the real-looking PEM with a clearly
synthetic placeholder (e.g., "REDACTED_SSH_PRIVATE_KEY" or
"mock-ssh-private-key") in both the object used to create the result and the
expect(...) assertion so result and expect still match but no PEM-like
header/footer appears; update the literal in the test that constructs the value
and in the expect(result!.ssh_private_key) comparison.
🪄 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: 02940c57-a7c0-4702-a153-089e1f17a2e1

📥 Commits

Reviewing files that changed from the base of the PR and between abcaa1d and 5d3a88b.

📒 Files selected for processing (1)
  • packages/opencode/test/altimate/ssh-tunnel-and-registry.test.ts

Comment on lines +4 to +5
beforeAll(() => { process.env.ALTIMATE_TELEMETRY_DISABLED = "true" })
afterAll(() => { delete process.env.ALTIMATE_TELEMETRY_DISABLED })
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Preserve and restore pre-existing telemetry env state.

Line 5 always deletes ALTIMATE_TELEMETRY_DISABLED, which can leak global state across suites if it was already set before this file ran. Save and restore the original value.

🔧 Proposed fix
 import { describe, test, expect, beforeAll, afterAll } from "bun:test"
 
 // Disable telemetry to avoid side-effects
-beforeAll(() => { process.env.ALTIMATE_TELEMETRY_DISABLED = "true" })
-afterAll(() => { delete process.env.ALTIMATE_TELEMETRY_DISABLED })
+const prevTelemetryDisabled = process.env.ALTIMATE_TELEMETRY_DISABLED
+beforeAll(() => { process.env.ALTIMATE_TELEMETRY_DISABLED = "true" })
+afterAll(() => {
+  if (prevTelemetryDisabled === undefined) {
+    delete process.env.ALTIMATE_TELEMETRY_DISABLED
+  } else {
+    process.env.ALTIMATE_TELEMETRY_DISABLED = prevTelemetryDisabled
+  }
+})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/altimate/ssh-tunnel-and-registry.test.ts` around lines
4 - 5, The test setup currently unconditionally sets and then deletes
ALTIMATE_TELEMETRY_DISABLED in beforeAll/afterAll; modify beforeAll to capture
the original value (e.g. const _orig = process.env.ALTIMATE_TELEMETRY_DISABLED),
then set process.env.ALTIMATE_TELEMETRY_DISABLED = "true", and modify afterAll
to restore the original: if (_orig === undefined) delete
process.env.ALTIMATE_TELEMETRY_DISABLED else
process.env.ALTIMATE_TELEMETRY_DISABLED = _orig; keep the references to the same
beforeAll and afterAll functions and the ALTIMATE_TELEMETRY_DISABLED env var so
the test no longer leaks global state.

Comment on lines +67 to +70
ssh_private_key: "-----BEGIN OPENSSH PRIVATE KEY-----\nAAA...",
})
expect(result).not.toBeNull()
expect(result!.ssh_private_key).toBe("-----BEGIN OPENSSH PRIVATE KEY-----\nAAA...")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Avoid PEM-formatted private-key literals in tests.

The string at Line 67/Line 70 matches a real private-key header pattern and can trigger secret scanners or policy gates. Use a clearly synthetic placeholder value instead.

🔐 Proposed fix
-      ssh_private_key: "-----BEGIN OPENSSH PRIVATE KEY-----\nAAA...",
+      ssh_private_key: "__TEST_SSH_PRIVATE_KEY__",
@@
-    expect(result!.ssh_private_key).toBe("-----BEGIN OPENSSH PRIVATE KEY-----\nAAA...")
+    expect(result!.ssh_private_key).toBe("__TEST_SSH_PRIVATE_KEY__")
📝 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
ssh_private_key: "-----BEGIN OPENSSH PRIVATE KEY-----\nAAA...",
})
expect(result).not.toBeNull()
expect(result!.ssh_private_key).toBe("-----BEGIN OPENSSH PRIVATE KEY-----\nAAA...")
ssh_private_key: "__TEST_SSH_PRIVATE_KEY__",
})
expect(result).not.toBeNull()
expect(result!.ssh_private_key).toBe("__TEST_SSH_PRIVATE_KEY__")
🧰 Tools
🪛 Betterleaks (1.1.1)

[high] 67-70: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.

(private-key)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/altimate/ssh-tunnel-and-registry.test.ts` around lines
67 - 70, The test currently embeds a PEM-formatted private key literal in the
assertions (the string compared to result!.ssh_private_key), which triggers
secret scanners; replace the real-looking PEM with a clearly synthetic
placeholder (e.g., "REDACTED_SSH_PRIVATE_KEY" or "mock-ssh-private-key") in both
the object used to create the result and the expect(...) assertion so result and
expect still match but no PEM-like header/footer appears; update the literal in
the test that constructs the value and in the expect(result!.ssh_private_key)
comparison.

anandgupta42 added a commit that referenced this pull request Mar 27, 2026
Deduplicate overlapping tests from PRs #494, #499, #502, #504, #506,
#507, #508, #510, #511, #512. Most MongoDB/env-var/dbt coverage was
already on main; this adds only genuinely new tests:

- SSH tunnel: `extractSshConfig` validation + lifecycle safety (7 tests)
- dbt profiles: spark->databricks, trino->postgres adapter mapping
- `dbtConnectionsToConfigs` conversion + empty input handling
- `containerToConfig` with fully-populated container
- MongoDB assertions in `telemetry-safety.test.ts`
- Sanity suite: branding, deny, driver, resilience expansions (#494)

Closes #513

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@anandgupta42
Copy link
Copy Markdown
Contributor Author

Consolidated into #514. This PR's test coverage was either already on main or has been included in the consolidated PR.

anandgupta42 added a commit that referenced this pull request Mar 27, 2026
…514)

* test: consolidate test coverage from 10 hourly PRs into single PR

Deduplicate overlapping tests from PRs #494, #499, #502, #504, #506,
#507, #508, #510, #511, #512. Most MongoDB/env-var/dbt coverage was
already on main; this adds only genuinely new tests:

- SSH tunnel: `extractSshConfig` validation + lifecycle safety (7 tests)
- dbt profiles: spark->databricks, trino->postgres adapter mapping
- `dbtConnectionsToConfigs` conversion + empty input handling
- `containerToConfig` with fully-populated container
- MongoDB assertions in `telemetry-safety.test.ts`
- Sanity suite: branding, deny, driver, resilience expansions (#494)

Closes #513

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address code review findings from 6-model consensus review

- Use `require.resolve()` instead of `require()` for driver resolvability
  checks to avoid false negatives from native binding load failures
- Remove unnecessary `altimate_change` markers from new ssh-tunnel test file
- Add `closeAllTunnels()` cleanup in `afterEach` to prevent state leaks

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address convergence review findings — driver warns, unshare guard

- Driver resolvability checks now emit warnings instead of failures
  since drivers are intentionally absent from sanity Docker image (#295)
- `unshare --net` now tests with a dry-run before use, falling back to
  proxy-based network blocking when unprivileged (macOS, containers)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants