test: keybind parsing & credential multi-field stripping

[anandgupta42]

What does this PR do?

Adds targeted unit tests for two untested areas discovered during automated test discovery, plus extends an existing test suite.

1. Keybind — src/util/keybind.ts (22 new tests)

This module is the TUI's entire keyboard shortcut system — parsing key strings like "ctrl+shift+a", matching parsed keys against each other, and formatting them for display. It had zero tests. A regression here silently breaks all keyboard shortcuts in the primary user interface.

New coverage includes:

• parse(): simple keys, modifier combinations (ctrl/alt/meta/option/shift/super), <leader> prefix, comma-separated multi-bindings, esc→escape normalization, "none" sentinel
• match(): identical key matching, undefined first-arg guard, super field normalization (missing → false), modifier mismatch rejection
• toString(): modifier ordering (ctrl+alt+super+shift), leader prefix formatting, delete→del mapping, undefined input
• fromParsedKey(): space normalization to "space", leader flag propagation, default leader=false

2. CredentialStore.saveConnection multi-field stripping — src/altimate/native/connections/credential-store.ts (1 new test)

Existing tests verified single-field stripping (password alone, private_key alone, OAuth alone). But real Snowflake configs have 7+ simultaneous sensitive fields (password, private_key, private_key_passphrase, token, oauth_client_secret, ssh_password, connection_string). If the iteration logic has a bug (early return, skipped field), credentials could leak to the config JSON file. The new test exercises the full multi-field path and verifies warning count matches.

3. containerToConfig conditional field omission — src/altimate/native/connections/docker-discovery.ts (1 new test)

The containerToConfig function converts auto-discovered Docker containers to ConnectionConfig. When optional fields (user, password, database) are undefined, they must not appear in the output config. The existing Docker discovery test only checked "returns empty when dockerode not installed". The new test verifies the conditional field omission behavior that users hit when Docker containers lack explicit credentials.

Type of change

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

Issue for this PR

N/A — proactive test coverage from automated test discovery

How did you verify your code works?

bun test test/util/keybind.test.ts          # 22 pass, 32 expect() calls
bun test test/altimate/connections.test.ts   # 41 pass (39 existing + 2 new), 97 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_01Lui6DcLFYL75nCttunEPpH

Summary by CodeRabbit

• Tests
  • Expanded Docker discovery and connection credential handling test coverage to verify proper sensitive field removal, preservation of non-sensitive fields, and corresponding warning generation during credential storage operations.
  • Added comprehensive keybinding utility test suite covering key parsing with modifier support, key matching logic, string formatting with proper modifier ordering, and parsed key normalization.

[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.}

[gitguardian]

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
29254673	Triggered	Generic Password	690303f	packages/opencode/test/altimate/connections.test.ts	View secret
29254674	Triggered	Generic Password	690303f	packages/opencode/test/altimate/connections.test.ts	View secret

🛠 Guidelines to remediate hardcoded secrets

1. Understand the implications of revoking this secret by investigating where it is used in your code.
2. Replace and store your secrets safely. Learn here the best practices.
3. Revoke and rotate these secrets.
4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

• following these best practices for managing and storing secrets including API keys and other credentials
• install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

---

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Comprehensive test coverage added for two utility modules: credential storage security (verifying removal of sensitive fields from connection configs) and keyboard binding parsing/matching functionality. No production code or public API changes.

Changes

Cohort / File(s)	Summary
Credential Security Tests packages/opencode/test/altimate/connections.test.ts	Updated imports for containerToConfig testing; added test verifying CredentialStore.saveConnection strips 7 sensitive fields (password, private_key, private_key_passphrase, token, oauth_client_secret, ssh_password, connection_string) while preserving non-sensitive fields and emitting warnings; added test ensuring containerToConfig omits optional fields when inputs are undefined.
Keybind Utility Tests packages/opencode/test/util/keybind.test.ts	New comprehensive test suite covering Keybind.parse (modifiers: ctrl, alt/meta/option, super, <leader>; comma-separated bindings; "esc"→"escape" normalization), Keybind.match (identifier matching, modifier comparison, super field normalization), Keybind.toString (modifier ordering, leader prefix formatting, "delete"→"del" mapping), and Keybind.fromParsedKey (space key normalization, leader flag handling).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

contributor

Poem

🐰 Tests bloom like clover in the spring,
Credentials guarded, keybinds ring,
With ctrl+alt and <leader> dance,
Security checks get their chance! 🌿

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the two main additions: keybind parsing tests and credential multi-field stripping tests, matching the core changes in the PR.
Description check	✅ Passed	The description provides comprehensive details on changes, testing approach, and verification, though it deviates from the standard template structure with a custom format.
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-0014

---

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/util/keybind.test.ts (1)

214-232: Optional: extract small test fixtures to reduce repeated object literals.

Line 216, Line 222, and Line 228 repeat the same parsed-key shape; a tiny factory would make updates safer and cleaner.

♻️ Suggested refactor

+const parsedKey = (overrides: Partial<{ name: string; ctrl: boolean; meta: boolean; shift: boolean; super: boolean }> = {}) => ({
+  name: "a",
+  ctrl: false,
+  meta: false,
+  shift: false,
+  super: false,
+  ...overrides,
+})
+
 describe("Keybind.fromParsedKey", () => {
   test("normalizes space to 'space'", () => {
-    const parsed = { name: " ", ctrl: false, meta: false, shift: false, super: false }
+    const parsed = parsedKey({ name: " " })
     const result = Keybind.fromParsedKey(parsed as any)
     expect(result.name).toBe("space")
   })
 
   test("sets leader flag when passed", () => {
-    const parsed = { name: "a", ctrl: false, meta: false, shift: false, super: false }
+    const parsed = parsedKey()
     const result = Keybind.fromParsedKey(parsed as any, true)
     expect(result.leader).toBe(true)
   })
 
   test("defaults leader to false", () => {
-    const parsed = { name: "a", ctrl: false, meta: false, shift: false, super: false }
+    const parsed = parsedKey()
     const result = Keybind.fromParsedKey(parsed as any)
     expect(result.leader).toBe(false)
   })
 })

🤖 Prompt for AI Agents

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

In `@packages/opencode/test/util/keybind.test.ts` around lines 214 - 232, Tests
for Keybind.fromParsedKey repeat the same parsed-key object literal; create a
small test fixture factory (e.g., makeParsedKey or parsedKeyFixture) used across
the three tests to produce the parsed object with defaults and an overridable
name or leader flag, then replace the repeated literals in the
Keybind.fromParsedKey tests with calls to that factory to reduce duplication and
make future changes safer.

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

131-131: Test name overstates coverage scope.

Line 131 says “all sensitive fields,” but this test validates a subset. Please rename to reflect “all provided sensitive fields” (or expand coverage) to avoid false confidence.

🤖 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` at line 131, Rename the
Jest test whose title is "saveConnection strips all sensitive fields from
complex config" to accurately state it only checks the provided subset (e.g.,
"saveConnection strips all provided sensitive fields from complex config") or
expand the test body to assert removal of every known sensitive key; locate the
test by the test(...) call at the top of the block and update the string title
(or add assertions for additional sensitive keys) so the name no longer
overstates coverage.

🤖 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/connections.test.ts`:
- Around line 326-329: The test currently checks optional fields with
expect(config.user).toBeUndefined(), which still passes if the keys exist but
are set to undefined; change the assertions to assert key omission directly by
replacing those checks with property-absence assertions (e.g., use
expect(config).not.toHaveProperty('user'),
expect(config).not.toHaveProperty('password'), and
expect(config).not.toHaveProperty('database')) so the test fails if the keys are
present even with undefined values.

---

Nitpick comments:
In `@packages/opencode/test/altimate/connections.test.ts`:
- Line 131: Rename the Jest test whose title is "saveConnection strips all
sensitive fields from complex config" to accurately state it only checks the
provided subset (e.g., "saveConnection strips all provided sensitive fields from
complex config") or expand the test body to assert removal of every known
sensitive key; locate the test by the test(...) call at the top of the block and
update the string title (or add assertions for additional sensitive keys) so the
name no longer overstates coverage.

In `@packages/opencode/test/util/keybind.test.ts`:
- Around line 214-232: Tests for Keybind.fromParsedKey repeat the same
parsed-key object literal; create a small test fixture factory (e.g.,
makeParsedKey or parsedKeyFixture) used across the three tests to produce the
parsed object with defaults and an overridable name or leader flag, then replace
the repeated literals in the Keybind.fromParsedKey tests with calls to that
factory to reduce duplication and make future changes safer.

🪄 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: c1fe9997-0186-4d8c-b784-30b940668f6d

📥 Commits

Reviewing files that changed from the base of the PR and between abcaa1d and 690303f.

📒 Files selected for processing (2)

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

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Assert key omission explicitly, not just undefined.

Lines 327-329 currently pass even if keys exist with undefined values. Since the behavior under test is omission, assert key absence directly.

Proposed test assertion fix

-    expect(config.user).toBeUndefined()
-    expect(config.password).toBeUndefined()
-    expect(config.database).toBeUndefined()
+    expect("user" in config).toBe(false)
+    expect("password" in config).toBe(false)
+    expect("database" in config).toBe(false)

📝 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

	// Optional fields should not be set when undefined
	expect(config.user).toBeUndefined()
	expect(config.password).toBeUndefined()
	expect(config.database).toBeUndefined()
	// Optional fields should not be set when undefined
	expect("user" in config).toBe(false)
	expect("password" in config).toBe(false)
	expect("database" in config).toBe(false)

🤖 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 326 - 329,
The test currently checks optional fields with
expect(config.user).toBeUndefined(), which still passes if the keys exist but
are set to undefined; change the assertions to assert key omission directly by
replacing those checks with property-absence assertions (e.g., use
expect(config).not.toHaveProperty('user'),
expect(config).not.toHaveProperty('password'), and
expect(config).not.toHaveProperty('database')) so the test fails if the keys are
present even with undefined values.

[anandgupta42]

Consolidated into #498. Tests deduplicated and merged into a single PR.