test: id + shell — monotonic ID ordering and shell blacklist enforcement

[anandgupta42]

What does this PR do?

Adds first-ever tests for two core infrastructure modules that had zero test coverage.

1. Identifier — src/id/id.ts (14 new tests)

This module generates all IDs in the system (sessions, messages, parts, permissions, etc.). It's used everywhere — session ordering, message sequencing, database operations, and cleanup cutoffs. Zero tests existed. New coverage includes:

• Prefix format correctness for all ID types, including the 4-char tool prefix outlier
• Ascending sort order: IDs with later timestamps sort greater in string order (critical invariant for message rendering)
• Descending sort order: IDs with later timestamps sort smaller (used for reverse-chronological views)
• Monotonic counter: Multiple IDs at the same millisecond are unique and correctly ordered
• Timestamp comparison: timestamp() preserves relative ordering between IDs (used by Truncate.cleanup() for retention cutoffs)
• Given-ID passthrough validation and wrong-prefix rejection
• Zod schema validation for both standard (3-char) and outlier (4-char tool) prefixes

2. Shell.acceptable and Shell.preferred — src/shell/shell.ts (9 new tests)

The Shell module selects which shell to use for bash tool execution. The acceptable() function blacklists fish and nu (Nushell) because they have incompatible syntax. If the blacklist fails, every bash tool call silently breaks for fish/nu users. Zero tests existed. New coverage includes:

• acceptable() returns $SHELL for non-blacklisted shells (bash, zsh)
• acceptable() rejects fish and nu, returning a valid fallback
• Basename-only matching: /opt/nushell/bin/bash is NOT blacklisted (only the basename matters)
• preferred() returns $SHELL even for blacklisted shells (intentional — used for user's terminal, not tool execution)
• Fallback behavior when $SHELL is unset (CI environments, containers)
• Proper lazy() cache reset between tests to prevent order-dependent flakiness

Type of change

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

Issue for this PR

N/A — proactive test coverage via /test-discovery

How did you verify your code works?

bun test test/id/id.test.ts        # 14 pass
bun test test/shell/shell.test.ts  #  9 pass

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_01SA2444fMWSyZaR3EkdJ7bL

Summary by CodeRabbit

• Tests
  • Added comprehensive test suites for identifier generation system, validating prefix support, lexicographic ordering, uniqueness, and timestamp handling across various scenarios.
  • Added test coverage for shell environment detection, verifying fallback behavior and preference handling across different configurations.

[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 suites were added: one validating Identifier class functionality for ID generation, ordering, uniqueness, and schema validation; another testing Shell class methods for acceptable and preferred shell resolution with environment variable handling and blacklist filtering.

Changes

Cohort / File(s)	Summary
Identifier Tests packages/opencode/test/id/id.test.ts	Test suite for ID prefix generation (ascending/descending), lexicographic ordering properties, uniqueness at same timestamp, timestamp extraction, passthrough behavior with given IDs, and schema validation with prefix matching.
Shell Tests packages/opencode/test/shell/shell.test.ts	Test suite for Shell.acceptable() with blacklist filtering and fallback behavior, Shell.preferred() without filtering, environment variable handling, cache management, and path validation across bash, zsh, fish, and nu shells.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Suggested labels

contributor

Poem

🐰 Hop, hop, test by test we go,
IDs ascending, shells at flow,
Blacklists banned, timestamps true,
Coverage blooms in morning dew! 🌱✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the main changes: adding tests for monotonic ID ordering and shell blacklist enforcement in two core modules.
Description check	✅ Passed	The description includes all required sections: a detailed summary of changes, test plan with verification commands, and a checklist with tests and documentation considerations marked complete.
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 claude/test-id-shell-01SA2444fMWSyZaR3EkdJ7bL

---

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

🤖 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/shell/shell.test.ts`:
- Around line 55-61: The test "returns fallback when SHELL is unset" calls
Shell.acceptable() and currently uses a regex requiring a leading forward slash
but also includes "cmd\.exe", which can never match Windows paths with
backslashes; update the test to either (a) limit the assertion to POSIX shells
by removing "cmd\.exe" from the regex, or (b) handle platforms explicitly by
branching on process.platform and asserting a POSIX-style regex
(/\/(bash|zsh|sh)$/) on 'linux'/'darwin' and a Windows-style regex (e.g.,
/\\(?:Windows\\)?(?:System32\\)?cmd\.exe$/ or checking for 'cmd.exe' via
/cmd\.exe$/) on 'win32'; modify the test for "returns fallback when SHELL is
unset" and reference Shell.acceptable() so it validates the correct path format
per platform.

🪄 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: 686f8770-5082-4133-8735-adf7639d1723

📥 Commits

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

📒 Files selected for processing (2)

• packages/opencode/test/id/id.test.ts
• packages/opencode/test/shell/shell.test.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Regex won't match Windows cmd.exe paths.

The comment on line 59 says "On Linux/macOS" but the regex includes cmd\.exe. Windows paths use backslashes (e.g., C:\Windows\System32\cmd.exe), so the pattern requiring a forward slash will never match cmd.exe.

Either remove cmd\.exe from the pattern to align with the comment, or adjust the regex to handle both path separators:

Option 1: Remove cmd.exe (match comment scope)

-    expect(result).toMatch(/\/(bash|zsh|sh|cmd\.exe)$/)
+    expect(result).toMatch(/\/(bash|zsh|sh)$/)

Option 2: Handle both separators

-    expect(result).toMatch(/\/(bash|zsh|sh|cmd\.exe)$/)
+    expect(result).toMatch(/[/\\](bash|zsh|sh|cmd\.exe)$/)

📝 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

	test("returns fallback when SHELL is unset", () => {
	delete process.env.SHELL
	const result = Shell.acceptable()
	expect(result.length).toBeGreaterThan(0)
	// On Linux/macOS, fallback should be a valid shell path
	expect(result).toMatch(/\/(bash|zsh|sh|cmd\.exe)$/)
	})
	test("returns fallback when SHELL is unset", () => {
	delete process.env.SHELL
	const result = Shell.acceptable()
	expect(result.length).toBeGreaterThan(0)
	// On Linux/macOS, fallback should be a valid shell path
	expect(result).toMatch(/[/\\](bash|zsh|sh|cmd\.exe)$/)
	})

🤖 Prompt for AI Agents

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

In `@packages/opencode/test/shell/shell.test.ts` around lines 55 - 61, The test
"returns fallback when SHELL is unset" calls Shell.acceptable() and currently
uses a regex requiring a leading forward slash but also includes "cmd\.exe",
which can never match Windows paths with backslashes; update the test to either
(a) limit the assertion to POSIX shells by removing "cmd\.exe" from the regex,
or (b) handle platforms explicitly by branching on process.platform and
asserting a POSIX-style regex (/\/(bash|zsh|sh)$/) on 'linux'/'darwin' and a
Windows-style regex (e.g., /\\(?:Windows\\)?(?:System32\\)?cmd\.exe$/ or
checking for 'cmd.exe' via /cmd\.exe$/) on 'win32'; modify the test for "returns
fallback when SHELL is unset" and reference Shell.acceptable() so it validates
the correct path format per platform.

[anandgupta42]

Consolidated into #403