fix: stabilize PR107 tmux spawn, offline buffering, and bun runtime#14
fix: stabilize PR107 tmux spawn, offline buffering, and bun runtime#14leeroybrun wants to merge 20 commits intomainfrom
Conversation
Validation: - Verified TMUX/TMUX_PANE values inside an isolated tmux server (custom socket) - Verified kill-window syntax and new-session -t semantics via tmux manpage Changes: - Correct TMUX env parsing (socket_path + server_pid) and prefer TMUX_PANE for pane id - Avoid invalid -t injection for commands that already specify a target; do not treat new-session as targetable - Fix kill-window invocation to use -t session:window - Align Machine.metadata typing with runtime (nullable) - Make socket send behavior consistent when disconnected; tighten misc scripts/tests
Remove the hard send guard that dropped messages while disconnected and rely on socket.io client buffering (tests updated).
Make debugLargeJson a true DEBUG-only path and avoid logging expanded env values/defaults; tighten doctor masking for default templates.
Align happy-cli profile schema with the app: profiles are env-var based only. Migrate any legacy provider config fields into environmentVariables during parsing to avoid data loss.
Add a single runtime invocation builder for node vs bun and allow overriding via HAPPY_CLI_SUBPROCESS_RUNTIME.
The GUI sends a profile-derived env var map that may include provider-specific keys and tmux knobs; type it as Record<string,string>.
📝 WalkthroughWalkthroughConsolidates env init, adds profileId plumbing and environment-preview RPC, migrates legacy profile configs to environmentVariables, refactors tmux utilities and daemon spawn paths, expands env-var expansion/masking, improves session/socket handling, and adds many tests across CLI, daemon, API, codex, tmux, persistence, and utils. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
Summary of ChangesHello @leeroybrun, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the stability, correctness, and security of the CLI's core functionalities, particularly around session management, environment variable handling, and integration with external tools like tmux and Bun. The changes aim to resolve several regressions and inconsistencies introduced in a previous update, ensuring a more robust and predictable user experience without exposing sensitive data. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a substantial and high-quality pull request that addresses several critical issues related to tmux integration, offline buffering, runtime support for Bun, and security. The fixes are well-implemented, and the extensive refactoring, especially in tmux.ts and persistence.ts, greatly improves the codebase's correctness and maintainability. The addition of comprehensive tests, including opt-in integration tests for tmux, is commendable. The improved logging and secret redaction are also significant enhancements that improve the security and debuggability of the application. Overall, this is an excellent set of changes that significantly stabilizes and hardens the CLI.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/claude/utils/sessionScanner.test.ts (1)
24-53: Usevi.stubEnvfor safe environment variable isolation in tests.
Manualprocess.envmutation and restoration can leak state across test workers or if exceptions occur. Replace with Vitest'svi.stubEnv()andvi.unstubAllEnvs()for proper test isolation.Import
vifrom vitest, remove theoriginalClaudeConfigDirvariable, replace the manual save/restore logic with:
vi.stubEnv('CLAUDE_CONFIG_DIR', claudeConfigDir)inbeforeEachvi.unstubAllEnvs()inafterEachSuggested changes
-import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'- let originalClaudeConfigDir: string | undefined let claudeConfigDir: string- originalClaudeConfigDir = process.env.CLAUDE_CONFIG_DIR; claudeConfigDir = join(testDir, 'claude-config'); - process.env.CLAUDE_CONFIG_DIR = claudeConfigDir; + vi.stubEnv('CLAUDE_CONFIG_DIR', claudeConfigDir);- if (originalClaudeConfigDir === undefined) { - delete process.env.CLAUDE_CONFIG_DIR; - } else { - process.env.CLAUDE_CONFIG_DIR = originalClaudeConfigDir; - } + vi.unstubAllEnvs();src/daemon/run.ts (1)
315-324: Protect auth token on disk with restrictive permissions.
auth.jsoncontains sensitive credentials. Writing without explicit mode can leave it world‑readable under permissive umasks.🔒 Suggested fix
- await fs.writeFile(join(codexHomeDir.name, 'auth.json'), options.token); + await fs.writeFile(join(codexHomeDir.name, 'auth.json'), options.token, { + mode: 0o600, + encoding: 'utf8', + });
🤖 Fix all issues with AI agents
In `@src/codex/codexMcpClient.test.ts`:
- Around line 5-43: Remove the test-suite mocks (the vi.mock calls for
'child_process' execSync, '@modelcontextprotocol/sdk/types.js'
ElicitRequestSchema, '@modelcontextprotocol/sdk/client/stdio.js'
StdioClientTransport and its __transportInstances, and the
'@modelcontextprotocol/sdk/client/index.js' Client) and convert the file into an
integration-style test that only runs when the real Codex CLI/transport is
available: implement a runtime check (e.g., attempt to run the Codex CLI version
or probe the actual transport binary) and skip the suite when the CLI is not
found, then exercise the real StdioClientTransport and Client paths so
assertions hit real execution (rather than mocked) code paths.
In `@src/daemon/run.tmuxEnv.test.ts`:
- Around line 1-9: Add a file-level JSDoc header comment and replace the dynamic
import inside the test with a static top-level import; specifically, move the
line that does const runModule = (await import('@/daemon/run'))... out of the
test and instead add import * as runModule from '@/daemon/run' (or the
appropriate named import) at the top, then reference
runModule.buildTmuxWindowEnv in the test to call the buildTmuxWindowEnv function
and keep the test otherwise unchanged.
In `@src/daemon/run.tmuxSpawn.test.ts`:
- Around line 1-13: Add the required JSDoc file header at the very top of this
test file and replace the dynamic import inside the test with a static import
placed before the describe block (e.g. import { buildTmuxSpawnConfig } from
'@/daemon/run' or import * as runModule from '@/daemon/run'); then update the
test to call buildTmuxSpawnConfig (or runModule.buildTmuxSpawnConfig) directly
instead of using await import, and remove the in-test dynamic import and any
related type assertions. Ensure the new static import is the top-most
non-comment content and the JSDoc header appears above it.
In `@src/utils/tmux.commandEnv.test.ts`:
- Around line 1-4: Add a brief JSDoc header comment at the top of the test file
and move the TmuxUtilities import to the module-level imports (replace the
dynamic import usage of TmuxUtilities in the test body). Remove the
vi.resetModules() call and the dynamic import that loads TmuxUtilities (the code
that calls import('.../tmux.utilities') or similar) and update any references to
use the statically imported TmuxUtilities symbol; ensure any test setup that
relied on module reset is adapted to work with the static import (e.g., reset
spies/mocks in beforeEach).
- Around line 11-41: Remove the artificial child_process.spawn mock (the
spawnMock/getLastSpawnCall hoisted block and the vi.mock('child_process', ...)
call) so tests exercise the real subprocess behavior; update the tmux.commandEnv
tests to call the real spawn (or move the test into the real tmux integration
suite) and, if needed, start a temporary tmux server within the test and assert
environment variable propagation (use the existing spawn-based helpers rather
than spawnMock and reference spawnMock/getLastSpawnCall only to remove them).
In `@src/utils/tmux.socketPath.test.ts`:
- Around line 1-4: Add a JSDoc header at the top of
src/utils/tmux.socketPath.test.ts describing the test file's purpose and any
relevant test-scope notes; then address the dynamic import of TmuxUtilities:
either move the import for TmuxUtilities to a top-level static import if test
isolation via vi.resetModules() (used at vi.resetModules()) is not needed, or
retain the dynamic import (const { TmuxUtilities } = await
import('@/utils/tmux')) but add a brief inline comment explaining that
vi.resetModules() precedes it and is required for test isolation so the import
must remain dynamic—ensure the code keeps vi.resetModules() immediately before
the dynamic import if you choose the latter.
- Around line 11-41: This test currently mocks child_process.spawn via the
hoisted spawnMock/getLastSpawnCall and vi.mock('child_process') which violates
unit-test policy; either remove the spawnMock/getLastSpawnCall block and the
vi.mock('child_process') call so the test uses the real child_process.spawn, or
move the test into the integration test suite that exercises TmuxUtilities (and
when moving, wrap it with the existing "skip if tmux unavailable" guard) so
socket-path behavior is verified end-to-end rather than by mocking.
🧹 Nitpick comments (13)
src/ui/logger.test.ts (1)
35-44: Consider settingDEBUGbefore the import for clarity.The test works correctly because
debugLargeJsonchecksprocess.env.DEBUGat call-time. However, settingDEBUGafter the import (line 37) but before the call (line 39) is slightly counterintuitive and could confuse future readers.♻️ Suggested reorder for clarity
it('writes to log file when DEBUG is set', async () => { + process.env.DEBUG = '1'; const { logger } = (await import('@/ui/logger')) as typeof import('@/ui/logger'); - process.env.DEBUG = '1'; logger.debugLargeJson('[TEST] debugLargeJson', { secret: 'value' });src/utils/expandEnvVars.test.ts (1)
161-171: Consider adding a cross-entry assignment propagation test.This test validates
:=assignment reuse within the same value. Based on the implementation inexpandEnvVars.ts,assignedEnvpersists across entries within a singleexpandEnvironmentVariablescall. A test verifying cross-entry propagation would strengthen coverage:it('propagates ${VAR:=default} assignments to subsequent entries', () => { const envVars = { FIRST: '${MISSING_VAR:=assigned-value}', SECOND: '${MISSING_VAR}', }; const result = expandEnvironmentVariables(envVars, {}); expect(result).toEqual({ FIRST: 'assigned-value', SECOND: 'assigned-value', }); });src/modules/common/registerCommonHandlers.previewEnv.test.ts (1)
13-39: Add an explicit return type forcreateTestRpcManager.
Aligns with the repo’s strict TS typing expectations.As per coding guidelines, ...♻️ Suggested typing update
-function createTestRpcManager(params?: { scopePrefix?: string }) { +function createTestRpcManager( + params?: { scopePrefix?: string } +): { + call: <TResponse, TRequest>(method: string, request: TRequest) => Promise<TResponse>; +} {src/persistence.ts (1)
197-198: Consider adding a JSDoc comment explaining schema version semantics.The inline note clarifies scope, but a brief JSDoc would help downstream maintainers understand when to bump this version.
📝 Suggested documentation improvement
// Settings schema version: Integer for overall Settings structure compatibility // Incremented when Settings structure changes (e.g., adding profiles array was v1→v2) // Used for migration logic in readSettings() -// NOTE: This is the schema for happy-cli's local settings file (not the Happy app's server-synced account settings). +/** + * Schema version for happy-cli's local settings file. + * Increment when the Settings structure changes (not for profile schema changes). + * v1 → v2: Added profiles array + * v2 → v3: Removed localEnvironmentVariables cache + */ export const SUPPORTED_SCHEMA_VERSION = 3;src/claude/utils/sessionScanner.test.ts (1)
6-8: Prefer@/alias for src imports.
Consider aligning the newgetProjectPathimport with the@/alias convention forsrcpaths. As per coding guidelines, ...♻️ Suggested change
-import { getProjectPath } from './path' +import { getProjectPath } from '@/claude/utils/path'src/codex/codexMcpClient.ts (1)
23-29: Broaden version parsing regex to handle known output formats.The current regex pattern doesn't match all known
codex --versionoutput formats. Based on actual CLI releases, the command outputs:
- "codex-cli 0.77.0" ✓ (matches current pattern)
- "@openai/codex, 0.79.0" ✗ (doesn't match—pattern expects "codex-cli|codex" followed by space)
Expanding the regex to handle the package scope and comma separator would prevent silent failures and incorrect fallback to
mcp-serverfor unparseable version strings. Consider allowing optional separators (space, comma) and package scopes:♻️ Suggested improvement
- const match = version.match(/\b(?:codex-cli|codex)\s+v?(\d+\.\d+\.\d+(?:-alpha\.\d+)?)\b/i); + const match = version.match(/(?:`@openai`\/)?codex(?:-cli)?\s*[,\s]\s*v?(\d+\.\d+\.\d+(?:-alpha\.\d+)?)/i);scripts/env-wrapper.cjs (1)
54-61: LGTM! Proper input validation added.Good addition to validate the command argument before proceeding. This prevents confusing errors when the user forgets to provide a command.
Optional: Consider extracting usage output to reduce duplication.
The usage/example output is duplicated between the variant validation (lines 42-50) and the command validation (lines 55-59).
♻️ Optional DRY refactor
+function printUsage() { + console.error('Usage: node scripts/env-wrapper.js <stable|dev> <command> [...args]'); + console.error(''); + console.error('Variants:'); + console.error(' stable - Production-ready version (data: ~/.happy/)'); + console.error(' dev - Development version (data: ~/.happy-dev/)'); + console.error(''); + console.error('Examples:'); + console.error(' node scripts/env-wrapper.js stable daemon start'); + console.error(' node scripts/env-wrapper.js dev auth login'); +} + if (!variant || !VARIANTS[variant]) { - console.error('Usage: node scripts/env-wrapper.js <stable|dev> <command> [...args]'); - console.error(''); - console.error('Variants:'); - console.error(' stable - Production-ready version (data: ~/.happy/)'); - console.error(' dev - Development version (data: ~/.happy-dev/)'); - console.error(''); - console.error('Examples:'); - console.error(' node scripts/env-wrapper.js stable daemon start'); - console.error(' node scripts/env-wrapper.js dev auth login'); + printUsage(); process.exit(1); } if (!command) { - console.error('Usage: node scripts/env-wrapper.js <stable|dev> <command> [...args]'); - console.error(''); - console.error('Examples:'); - console.error(' node scripts/env-wrapper.js stable daemon start'); - console.error(' node scripts/env-wrapper.js dev auth login'); + printUsage(); process.exit(1); }src/utils/spawnHappyCLI.ts (1)
65-87: LGTM! Well-structured invocation builder.Good extraction of the subprocess invocation logic into a reusable, testable function. The design:
- Correctly applies
--no-warnings --no-deprecationonly for Node.js (these are Node-specific flags)- Validates entrypoint existence before returning
- Returns a typed tuple enabling callers to inspect the runtime choice
Optional: Consider reordering existence check before building argv.
Minor suggestion for logical flow clarity:
♻️ Optional reorder
export function buildHappyCliSubprocessInvocation(args: string[]): { runtime: 'node' | 'bun'; argv: string[] } { const projectRoot = projectPath(); const entrypoint = join(projectRoot, 'dist', 'index.mjs'); + // Sanity check of the entrypoint path exists + if (!existsSync(entrypoint)) { + const errorMessage = `Entrypoint ${entrypoint} does not exist`; + logger.debug(`[SPAWN HAPPY CLI] ${errorMessage}`); + throw new Error(errorMessage); + } + // Use the same Node.js flags that the wrapper script uses const nodeArgs = [ '--no-warnings', '--no-deprecation', entrypoint, ...args ]; - // Sanity check of the entrypoint path exists - if (!existsSync(entrypoint)) { - const errorMessage = `Entrypoint ${entrypoint} does not exist`; - logger.debug(`[SPAWN HAPPY CLI] ${errorMessage}`); - throw new Error(errorMessage); - } - const runtime = getSubprocessRuntime(); const argv = runtime === 'node' ? nodeArgs : [entrypoint, ...args]; return { runtime, argv }; }src/utils/tmux.real.integration.test.ts (1)
136-146: Prefer logger over console for debug cleanup output.Console debug output in src/** tests conflicts with the logging guidance. Consider switching to the shared logger for these diagnostic messages.
As per coding guidelines, src/** logging should go through the logger.♻️ Suggested change
-import { TmuxUtilities } from '@/utils/tmux'; +import { TmuxUtilities } from '@/utils/tmux'; +import { logger } from '@/ui/logger'; @@ - console.error('[tmux-it] Failed to kill isolated tmux server', { + logger.debug('[tmux-it] Failed to kill isolated tmux server', { socketPath, status: result.status, stderr: result.stderr, error: result.error?.message, }); @@ - console.error('[tmux-it] Failed to kill isolated tmux server via TMUX_TMPDIR', { + logger.debug('[tmux-it] Failed to kill isolated tmux server via TMUX_TMPDIR', { tmuxTmpDir, status: result.status, stderr: result.stderr, error: result.error?.message, });Also applies to: 309-316
src/utils/tmux.test.ts (3)
9-21: Use@/alias for src imports.Please align the tmux import with the repo alias convention for src paths.
As per coding guidelines, src/** imports should use the `@/` alias.♻️ Suggested change
-import { +import { normalizeExitCode, parseTmuxSessionIdentifier, formatTmuxSessionIdentifier, validateTmuxSessionIdentifier, buildTmuxSessionIdentifier, createTmuxSession, TmuxSessionIdentifierError, extractSessionAndWindow, TmuxUtilities, type TmuxSessionIdentifier, type TmuxCommandResult, -} from './tmux'; +} from '@/utils/tmux';
309-316: Add an explicit return type towithTmuxEnv.This helper should declare its return type explicitly for strict typing.
As per coding guidelines, TypeScript helpers should use explicit return types.♻️ Suggested change
- const withTmuxEnv = (value: string | undefined, fn: () => void, pane?: string | undefined) => { + const withTmuxEnv = (value: string | undefined, fn: () => void, pane?: string | undefined): void => {
485-620: Avoid mocking/spies in tests per repo policy.The new
vi.spyOnandFakeTmuxUtilitiesusage introduces mocking. Please refactor to use real tmux integration coverage (or reuse the opt‑in real‑tmux suite) instead of mocks.As per coding guidelines, tests should avoid mocking and make real API calls.
src/daemon/run.ts (1)
36-82: Add JSDoc for new exported helpers.These exported helpers should include JSDoc to document purpose, inputs, and outputs.
As per coding guidelines, new exports should carry JSDoc.♻️ Suggested change
-export function buildTmuxWindowEnv( +/** + * Build the environment to pass into a tmux window, merging daemon env and extra overrides. + */ +export function buildTmuxWindowEnv( daemonEnv: NodeJS.ProcessEnv, extraEnv: Record<string, string>, ): Record<string, string> { @@ -export function buildTmuxSpawnConfig(params: { +/** + * Build the tmux spawn command tokens and environment for the given agent + directory. + */ +export function buildTmuxSpawnConfig(params: {
| // NOTE: This test suite uses mocks because the real Codex CLI / MCP transport | ||
| // is not guaranteed to be available in CI or local test environments. | ||
| vi.mock('child_process', () => ({ | ||
| execSync: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock('@modelcontextprotocol/sdk/types.js', () => ({ | ||
| ElicitRequestSchema: {}, | ||
| })); | ||
|
|
||
| vi.mock('@modelcontextprotocol/sdk/client/stdio.js', () => { | ||
| const instances: any[] = []; | ||
|
|
||
| class StdioClientTransport { | ||
| public command: string; | ||
| public args: string[]; | ||
| public env: Record<string, string>; | ||
|
|
||
| constructor(opts: { command: string; args: string[]; env: Record<string, string> }) { | ||
| this.command = opts.command; | ||
| this.args = opts.args; | ||
| this.env = opts.env; | ||
| instances.push(this); | ||
| } | ||
| } | ||
|
|
||
| return { StdioClientTransport, __transportInstances: instances }; | ||
| }); | ||
|
|
||
| vi.mock('@modelcontextprotocol/sdk/client/index.js', () => { | ||
| class Client { | ||
| setNotificationHandler() { } | ||
| setRequestHandler() { } | ||
| async connect() { } | ||
| async close() { } | ||
| } | ||
|
|
||
| return { Client }; | ||
| }); |
There was a problem hiding this comment.
Avoid mocks in Vitest tests per project rules.
The test guidelines require real API calls and no mocking; these vi.mock blocks violate that and reduce confidence in CLI/transport behavior. Consider converting these to integration tests that run only when Codex CLI is available (skip otherwise) so the assertions still exercise real execution paths. Based on learnings.
🤖 Prompt for AI Agents
In `@src/codex/codexMcpClient.test.ts` around lines 5 - 43, Remove the test-suite
mocks (the vi.mock calls for 'child_process' execSync,
'@modelcontextprotocol/sdk/types.js' ElicitRequestSchema,
'@modelcontextprotocol/sdk/client/stdio.js' StdioClientTransport and its
__transportInstances, and the '@modelcontextprotocol/sdk/client/index.js'
Client) and convert the file into an integration-style test that only runs when
the real Codex CLI/transport is available: implement a runtime check (e.g.,
attempt to run the Codex CLI version or probe the actual transport binary) and
skip the suite when the CLI is not found, then exercise the real
StdioClientTransport and Client paths so assertions hit real execution (rather
than mocked) code paths.
| import { describe, it, expect } from 'vitest'; | ||
|
|
||
| describe('daemon tmux env building', () => { | ||
| it('merges daemon process env and profile env for tmux windows', async () => { | ||
| const runModule = (await import('@/daemon/run')) as typeof import('@/daemon/run'); | ||
| const merged = runModule.buildTmuxWindowEnv( | ||
| { PATH: '/bin', HOME: '/home/user', UNDEFINED: undefined }, | ||
| { HOME: '/override', CUSTOM: 'x' } | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the test file exists
ls -la src/daemon/run.tmuxEnv.test.ts 2>&1 || echo "File not found"Repository: leeroybrun/happy-cli
Length of output: 142
🏁 Script executed:
# Check if buildTmuxWindowEnv is exported from src/daemon/run.ts
rg "export.*buildTmuxWindowEnv|function buildTmuxWindowEnv" src/daemon/run.tsRepository: leeroybrun/happy-cli
Length of output: 99
🏁 Script executed:
# Get the full test file to verify current state
cat -n src/daemon/run.tmuxEnv.test.tsRepository: leeroybrun/happy-cli
Length of output: 826
Add file header JSDoc and use static imports at the top.
This test file is missing a header comment and violates the import guidelines by using a dynamic import inside the test function. Move the import to the top of the file as a static import.
♻️ Suggested update
+/**
+ * Tests tmux window environment merging for daemon sessions.
+ */
+import { buildTmuxWindowEnv } from '@/daemon/run';
import { describe, it, expect } from 'vitest';
@@
- const runModule = (await import('@/daemon/run')) as typeof import('@/daemon/run');
- const merged = runModule.buildTmuxWindowEnv(
+ const merged = buildTmuxWindowEnv(
{ PATH: '/bin', HOME: '/home/user', UNDEFINED: undefined },
{ HOME: '/override', CUSTOM: 'x' }
);📝 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.
| import { describe, it, expect } from 'vitest'; | |
| describe('daemon tmux env building', () => { | |
| it('merges daemon process env and profile env for tmux windows', async () => { | |
| const runModule = (await import('@/daemon/run')) as typeof import('@/daemon/run'); | |
| const merged = runModule.buildTmuxWindowEnv( | |
| { PATH: '/bin', HOME: '/home/user', UNDEFINED: undefined }, | |
| { HOME: '/override', CUSTOM: 'x' } | |
| ); | |
| /** | |
| * Tests tmux window environment merging for daemon sessions. | |
| */ | |
| import { buildTmuxWindowEnv } from '@/daemon/run'; | |
| import { describe, it, expect } from 'vitest'; | |
| describe('daemon tmux env building', () => { | |
| it('merges daemon process env and profile env for tmux windows', async () => { | |
| const merged = buildTmuxWindowEnv( | |
| { PATH: '/bin', HOME: '/home/user', UNDEFINED: undefined }, | |
| { HOME: '/override', CUSTOM: 'x' } | |
| ); |
🤖 Prompt for AI Agents
In `@src/daemon/run.tmuxEnv.test.ts` around lines 1 - 9, Add a file-level JSDoc
header comment and replace the dynamic import inside the test with a static
top-level import; specifically, move the line that does const runModule = (await
import('@/daemon/run'))... out of the test and instead add import * as runModule
from '@/daemon/run' (or the appropriate named import) at the top, then reference
runModule.buildTmuxWindowEnv in the test to call the buildTmuxWindowEnv function
and keep the test otherwise unchanged.
| import { describe, it, expect } from 'vitest'; | ||
|
|
||
| describe('daemon tmux spawn config', () => { | ||
| const originalRuntimeOverride = process.env.HAPPY_CLI_SUBPROCESS_RUNTIME; | ||
| const originalPath = process.env.PATH; | ||
|
|
||
| it('uses merged env and bun runtime when configured', async () => { | ||
| process.env.HAPPY_CLI_SUBPROCESS_RUNTIME = 'bun'; | ||
| process.env.PATH = '/bin'; | ||
|
|
||
| try { | ||
| const runModule = (await import('@/daemon/run')) as typeof import('@/daemon/run'); | ||
| const cfg = runModule.buildTmuxSpawnConfig({ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read its content
if [ -f "src/daemon/run.tmuxSpawn.test.ts" ]; then
echo "=== File exists, reading content ==="
cat -n "src/daemon/run.tmuxSpawn.test.ts"
else
echo "File not found at src/daemon/run.tmuxSpawn.test.ts"
# Try to find similar test files
fd -e "test.ts" "daemon" | head -20
fiRepository: leeroybrun/happy-cli
Length of output: 1821
🏁 Script executed:
# Check the run.ts file to see if buildTmuxSpawnConfig is exported
if [ -f "src/daemon/run.ts" ]; then
echo "=== Checking for buildTmuxSpawnConfig export ==="
rg "export.*buildTmuxSpawnConfig" src/daemon/run.ts -A 2
fiRepository: leeroybrun/happy-cli
Length of output: 220
🏁 Script executed:
# Verify the import path works
if [ -f "src/daemon/run.ts" ]; then
echo "=== Checking module exports ==="
head -50 src/daemon/run.ts | cat -n
fiRepository: leeroybrun/happy-cli
Length of output: 2502
Add JSDoc file header and move import to top of file.
This test file is missing the required JSDoc header comment and violates the guideline that all imports must be at the top. Move the import statement before the describe block and remove the dynamic import from within the test.
♻️ Suggested update
+/**
+ * Tests daemon tmux spawn config construction.
+ */
+import { buildTmuxSpawnConfig } from '@/daemon/run';
import { describe, it, expect } from 'vitest';
describe('daemon tmux spawn config', () => {
@@ -11,8 +15,7 @@ describe('daemon tmux spawn config', () => {
try {
- const runModule = (await import('@/daemon/run')) as typeof import('@/daemon/run');
- const cfg = runModule.buildTmuxSpawnConfig({
+ const cfg = buildTmuxSpawnConfig({
agent: 'claude',
directory: '/tmp',📝 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.
| import { describe, it, expect } from 'vitest'; | |
| describe('daemon tmux spawn config', () => { | |
| const originalRuntimeOverride = process.env.HAPPY_CLI_SUBPROCESS_RUNTIME; | |
| const originalPath = process.env.PATH; | |
| it('uses merged env and bun runtime when configured', async () => { | |
| process.env.HAPPY_CLI_SUBPROCESS_RUNTIME = 'bun'; | |
| process.env.PATH = '/bin'; | |
| try { | |
| const runModule = (await import('@/daemon/run')) as typeof import('@/daemon/run'); | |
| const cfg = runModule.buildTmuxSpawnConfig({ | |
| /** | |
| * Tests daemon tmux spawn config construction. | |
| */ | |
| import { buildTmuxSpawnConfig } from '@/daemon/run'; | |
| import { describe, it, expect } from 'vitest'; | |
| describe('daemon tmux spawn config', () => { | |
| const originalRuntimeOverride = process.env.HAPPY_CLI_SUBPROCESS_RUNTIME; | |
| const originalPath = process.env.PATH; | |
| it('uses merged env and bun runtime when configured', async () => { | |
| process.env.HAPPY_CLI_SUBPROCESS_RUNTIME = 'bun'; | |
| process.env.PATH = '/bin'; | |
| try { | |
| const cfg = buildTmuxSpawnConfig({ |
🤖 Prompt for AI Agents
In `@src/daemon/run.tmuxSpawn.test.ts` around lines 1 - 13, Add the required JSDoc
file header at the very top of this test file and replace the dynamic import
inside the test with a static import placed before the describe block (e.g.
import { buildTmuxSpawnConfig } from '@/daemon/run' or import * as runModule
from '@/daemon/run'); then update the test to call buildTmuxSpawnConfig (or
runModule.buildTmuxSpawnConfig) directly instead of using await import, and
remove the in-test dynamic import and any related type assertions. Ensure the
new static import is the top-most non-comment content and the JSDoc header
appears above it.
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import { EventEmitter } from 'node:events'; | ||
| import type { SpawnOptions, ChildProcessWithoutNullStreams } from 'node:child_process'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -70 src/utils/tmux.commandEnv.test.ts | cat -nRepository: leeroybrun/happy-cli
Length of output: 2255
🏁 Script executed:
fd -e test.ts src/ | head -5 | xargs -I {} head -15 {}Repository: leeroybrun/happy-cli
Length of output: 385
🏁 Script executed:
fd --full-path 'src/.*\.test\.ts$' | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -20 {}'Repository: leeroybrun/happy-cli
Length of output: 3364
🏁 Script executed:
cd src && find . -name "*.test.ts" -exec grep -l "JSDoc\|\/\*\*" {} \; | wc -l && find . -name "*.test.ts" | wc -lRepository: leeroybrun/happy-cli
Length of output: 69
🏁 Script executed:
cd src && find . -name "*.test.ts" -exec grep -l "/\*\*" {} \; | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && head -10 {}'Repository: leeroybrun/happy-cli
Length of output: 1720
🏁 Script executed:
cat CLAUDE.md 2>/dev/null || echo "File not found"Repository: leeroybrun/happy-cli
Length of output: 8628
Add a file header JSDoc and move TmuxUtilities import to the top.
This file should include a brief header comment and import TmuxUtilities statically at the top. Remove the vi.resetModules() call and dynamic import at line 49-50, keeping all imports at the module level as required by the coding guidelines.
♻️ Suggested update
+/**
+ * Tests tmux subprocess environment handling in TmuxUtilities.
+ */
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { EventEmitter } from 'node:events';
import type { SpawnOptions, ChildProcessWithoutNullStreams } from 'node:child_process';
+import { TmuxUtilities } from '@/utils/tmux';
@@
- vi.resetModules();
- const { TmuxUtilities } = await import('@/utils/tmux');
-
const utils = new TmuxUtilities('happy', { TMUX_TMPDIR: '/custom/tmux' });
await utils.executeTmuxCommand(['list-sessions']);🤖 Prompt for AI Agents
In `@src/utils/tmux.commandEnv.test.ts` around lines 1 - 4, Add a brief JSDoc
header comment at the top of the test file and move the TmuxUtilities import to
the module-level imports (replace the dynamic import usage of TmuxUtilities in
the test body). Remove the vi.resetModules() call and the dynamic import that
loads TmuxUtilities (the code that calls import('.../tmux.utilities') or
similar) and update any references to use the statically imported TmuxUtilities
symbol; ensure any test setup that relied on module reset is adapted to work
with the static import (e.g., reset spies/mocks in beforeEach).
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import { EventEmitter } from 'node:events'; | ||
| import type { SpawnOptions, ChildProcessWithoutNullStreams } from 'node:child_process'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/utils/tmux.socketPath.test.tsRepository: leeroybrun/happy-cli
Length of output: 184
🏁 Script executed:
cat -n src/utils/tmux.socketPath.test.tsRepository: leeroybrun/happy-cli
Length of output: 2320
Add a file header JSDoc comment.
This test file is missing the required header comment explaining its purpose. Additionally, the dynamic import at line 50 (const { TmuxUtilities } = await import('@/utils/tmux')) violates the guideline that ALL imports must be at the top of the file. However, note that it's preceded by vi.resetModules() at line 49, which is a legitimate test isolation pattern. If moving to a static top-level import, ensure the module reset behavior isn't needed for test correctness.
Suggested update
+/**
+ * Tests tmux socket-path usage in TmuxUtilities.
+ */
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { EventEmitter } from 'node:events';
import type { SpawnOptions, ChildProcessWithoutNullStreams } from 'node:child_process';
+import { TmuxUtilities } from '@/utils/tmux';📝 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.
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | |
| import { EventEmitter } from 'node:events'; | |
| import type { SpawnOptions, ChildProcessWithoutNullStreams } from 'node:child_process'; | |
| /** | |
| * Tests tmux socket-path usage in TmuxUtilities. | |
| */ | |
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | |
| import { EventEmitter } from 'node:events'; | |
| import type { SpawnOptions, ChildProcessWithoutNullStreams } from 'node:child_process'; | |
| import { TmuxUtilities } from '@/utils/tmux'; |
🤖 Prompt for AI Agents
In `@src/utils/tmux.socketPath.test.ts` around lines 1 - 4, Add a JSDoc header at
the top of src/utils/tmux.socketPath.test.ts describing the test file's purpose
and any relevant test-scope notes; then address the dynamic import of
TmuxUtilities: either move the import for TmuxUtilities to a top-level static
import if test isolation via vi.resetModules() (used at vi.resetModules()) is
not needed, or retain the dynamic import (const { TmuxUtilities } = await
import('@/utils/tmux')) but add a brief inline comment explaining that
vi.resetModules() precedes it and is required for test isolation so the import
must remain dynamic—ensure the code keeps vi.resetModules() immediately before
the dynamic import if you choose the latter.
Handle codex --version output variations without misreporting 'not installed' and remove stdout logging of elicitation payloads.
Provide an EventEmitter-compatible stub and a focused unit test so offline mode can't crash on basic session events.
- Treat TMUX_TMPDIR as a directory via tmux client env (not -S socket path) - Pass per-window env via new-window -e KEY=value without shell-style quoting - Make empty sessionName resolve deterministically (attached/most-recent) - Preserve failure semantics for signal-terminated tmux commands
leeroybrun
force-pushed
the
slopus/pr/pr107-followup-fixes
branch
from
df67085 to
0ff0b37
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/modules/common/registerCommonHandlers.ts`:
- Around line 351-362: The code builds a RegExp from the environment value
HAPPY_ENV_PREVIEW_SECRET_NAME_REGEX (overrideRegexRaw) which risks ReDoS; update
secretNameRegex creation to validate and sanitize the user-provided pattern
before constructing a RegExp: reject or fallback to defaultSecretNameRegex if
the string is unusually long (e.g., >200 chars), contains potentially dangerous
constructs (e.g., nested quantifiers like (?:.*+){n} or catastrophic
backtracking patterns), or fails a safety check from a vetted library (e.g.,
safe-regex); keep the existing try/catch but add the pre-check on
overrideRegexRaw/trimmed and only construct new RegExp when the safety check
passes, otherwise log a warning and return defaultSecretNameRegex (affecting
variables: overrideRegexRaw, trimmed, secretNameRegex, defaultSecretNameRegex).
In `@src/utils/tmux.test.ts`:
- Around line 8-21: The tests for createTmuxSession and spawnInTmux currently
use vi.spyOn and a FakeTmuxUtilities subclass to stub tmux behavior; remove
those mocks and rewrite the tests to either invoke real tmux via the actual
TmuxUtilities implementation (so they exercise createTmuxSession and spawnInTmux
through real subprocess calls) or move these test cases into a separate
integration test suite and mark them accordingly; specifically eliminate usage
of vi.spyOn(...) and the FakeTmuxUtilities class, update references to
createTmuxSession and spawnInTmux to use the real TmuxUtilities, and ensure any
environment/setup/teardown needed for real tmux subprocesses is performed in the
test harness.
♻️ Duplicate comments (3)
src/codex/codexMcpClient.test.ts (1)
5-43: Replace mocks with integration-style Vitest tests per project rules.Project guidance requires real API calls and no mocking; these
vi.mockblocks violate that and can mask regressions. Convert to an integration-style suite that skips when Codex CLI/transport is unavailable. Based on learnings.src/utils/tmux.commandEnv.test.ts (2)
1-4: Add a file header JSDoc and keep imports static.The dynamic import plus
vi.resetModules()conflicts with the “imports at top” rule, and the file still lacks a header comment. Prefer a module‑levelTmuxUtilitiesimport and reset spies/mocks inbeforeEachif needed.♻️ Proposed update
+/** + * Tests tmux subprocess environment handling. + */ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { EventEmitter } from 'node:events'; import type { SpawnOptions, ChildProcessWithoutNullStreams } from 'node:child_process'; +import { TmuxUtilities } from '@/utils/tmux'; @@ - vi.resetModules(); - const { TmuxUtilities } = await import('@/utils/tmux'); - const utils = new TmuxUtilities('happy', { TMUX_TMPDIR: '/custom/tmux' });As per coding guidelines, please keep imports at the top and add a file header JSDoc.
Also applies to: 49-50
11-41: Avoid mockingchild_process.spawnin tests.This uses
vi.mock/vi.fnto stub subprocess behavior, but the guidelines require real API calls with no mocking. The opt‑in tmux integration suite already covers TMUX_TMPDIR propagation, so consider moving this assertion there or exercising a real tmux server.As per coding guidelines, unit tests should not mock subprocess APIs.
🧹 Nitpick comments (2)
src/codex/codexMcpClient.ts (2)
24-29: Validate fallback tomcp-serverfor unparseable Codex version output.Defaulting to
mcp-serverwhen the version format is unexpected could break older Codex installs that only supportmcp. Please verify this behavior against real CLI outputs, and consider a runtime fallback (e.g., trymcp-server, thenmcpif spawn fails).
52-120: Preserve denial context from permission handler (if available).If
handleToolCallcan return areason, consider threading it through so callers can surface meaningful denial/abort context instead of dropping it.
| const defaultSecretNameRegex = /TOKEN|KEY|SECRET|AUTH|PASS|PASSWORD|COOKIE/i; | ||
| const overrideRegexRaw = process.env.HAPPY_ENV_PREVIEW_SECRET_NAME_REGEX; | ||
| const secretNameRegex = (() => { | ||
| if (typeof overrideRegexRaw !== 'string') return defaultSecretNameRegex; | ||
| const trimmed = overrideRegexRaw.trim(); | ||
| if (!trimmed) return defaultSecretNameRegex; | ||
| try { | ||
| return new RegExp(trimmed, 'i'); | ||
| } catch { | ||
| return defaultSecretNameRegex; | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Potential ReDoS vulnerability with user-controlled regex.
The static analysis correctly flags that constructing a RegExp from HAPPY_ENV_PREVIEW_SECRET_NAME_REGEX environment variable could enable Regular Expression Denial of Service if a malicious pattern is provided.
While this is an environment variable (requiring daemon access to set), consider adding basic safety measures:
🛡️ Suggested mitigation
const secretNameRegex = (() => {
if (typeof overrideRegexRaw !== 'string') return defaultSecretNameRegex;
const trimmed = overrideRegexRaw.trim();
if (!trimmed) return defaultSecretNameRegex;
+ // Limit regex length to mitigate ReDoS risk
+ if (trimmed.length > 200) {
+ logger.warn('[preview-env] HAPPY_ENV_PREVIEW_SECRET_NAME_REGEX too long, using default');
+ return defaultSecretNameRegex;
+ }
try {
return new RegExp(trimmed, 'i');
} catch {
return defaultSecretNameRegex;
}
})();Alternatively, you could use a library like safe-regex to validate the pattern, though the risk is lower since this requires daemon-level access to exploit.
📝 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.
| const defaultSecretNameRegex = /TOKEN|KEY|SECRET|AUTH|PASS|PASSWORD|COOKIE/i; | |
| const overrideRegexRaw = process.env.HAPPY_ENV_PREVIEW_SECRET_NAME_REGEX; | |
| const secretNameRegex = (() => { | |
| if (typeof overrideRegexRaw !== 'string') return defaultSecretNameRegex; | |
| const trimmed = overrideRegexRaw.trim(); | |
| if (!trimmed) return defaultSecretNameRegex; | |
| try { | |
| return new RegExp(trimmed, 'i'); | |
| } catch { | |
| return defaultSecretNameRegex; | |
| } | |
| })(); | |
| const defaultSecretNameRegex = /TOKEN|KEY|SECRET|AUTH|PASS|PASSWORD|COOKIE/i; | |
| const overrideRegexRaw = process.env.HAPPY_ENV_PREVIEW_SECRET_NAME_REGEX; | |
| const secretNameRegex = (() => { | |
| if (typeof overrideRegexRaw !== 'string') return defaultSecretNameRegex; | |
| const trimmed = overrideRegexRaw.trim(); | |
| if (!trimmed) return defaultSecretNameRegex; | |
| // Limit regex length to mitigate ReDoS risk | |
| if (trimmed.length > 200) { | |
| logger.warn('[preview-env] HAPPY_ENV_PREVIEW_SECRET_NAME_REGEX too long, using default'); | |
| return defaultSecretNameRegex; | |
| } | |
| try { | |
| return new RegExp(trimmed, 'i'); | |
| } catch { | |
| return defaultSecretNameRegex; | |
| } | |
| })(); |
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 357-357: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(trimmed, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In `@src/modules/common/registerCommonHandlers.ts` around lines 351 - 362, The
code builds a RegExp from the environment value
HAPPY_ENV_PREVIEW_SECRET_NAME_REGEX (overrideRegexRaw) which risks ReDoS; update
secretNameRegex creation to validate and sanitize the user-provided pattern
before constructing a RegExp: reject or fallback to defaultSecretNameRegex if
the string is unusually long (e.g., >200 chars), contains potentially dangerous
constructs (e.g., nested quantifiers like (?:.*+){n} or catastrophic
backtracking patterns), or fails a safety check from a vetted library (e.g.,
safe-regex); keep the existing try/catch but add the pre-check on
overrideRegexRaw/trimmed and only construct new RegExp when the safety check
passes, otherwise log a warning and return defaultSecretNameRegex (affecting
variables: overrideRegexRaw, trimmed, secretNameRegex, defaultSecretNameRegex).
| import { describe, expect, it, vi } from 'vitest'; | ||
| import { | ||
| normalizeExitCode, | ||
| parseTmuxSessionIdentifier, | ||
| formatTmuxSessionIdentifier, | ||
| validateTmuxSessionIdentifier, | ||
| buildTmuxSessionIdentifier, | ||
| createTmuxSession, | ||
| TmuxSessionIdentifierError, | ||
| extractSessionAndWindow, | ||
| TmuxUtilities, | ||
| type TmuxSessionIdentifier, | ||
| type TmuxCommandResult, | ||
| } from './tmux'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/utils/tmux.test.tsRepository: leeroybrun/happy-cli
Length of output: 90
🏁 Script executed:
cat -n src/utils/tmux.test.tsRepository: leeroybrun/happy-cli
Length of output: 27932
Remove mocking from createTmuxSession and spawnInTmux tests to comply with the no-mocking guideline.
Tests for createTmuxSession (lines 486–498) and spawnInTmux (lines 501–621) violate the requirement to write unit tests without mocking. The vi.spyOn at line 487 and the FakeTmuxUtilities subclass at line 502 stub tmux execution paths instead of making real API calls. Use actual tmux subprocess execution in tests or move these to an integration suite.
🤖 Prompt for AI Agents
In `@src/utils/tmux.test.ts` around lines 8 - 21, The tests for createTmuxSession
and spawnInTmux currently use vi.spyOn and a FakeTmuxUtilities subclass to stub
tmux behavior; remove those mocks and rewrite the tests to either invoke real
tmux via the actual TmuxUtilities implementation (so they exercise
createTmuxSession and spawnInTmux through real subprocess calls) or move these
test cases into a separate integration test suite and mark them accordingly;
specifically eliminate usage of vi.spyOn(...) and the FakeTmuxUtilities class,
update references to createTmuxSession and spawnInTmux to use the real
TmuxUtilities, and ensure any environment/setup/teardown needed for real tmux
subprocesses is performed in the test harness.
|
I'm glad to see progress on outstanding bugs! Sorry about the log var leakage. I'm a bit surprised by the ai code review claim that env variables were nondeterministic. I had intended to set it up so if it is not assigned it uses the remote value and shows it to you, and if you want to override just set the value. That seemed deterministic in concept to me but maybe there was a bug? Or perhaps in the sense thay on a second machine with different variables set the outcome is different? I've also got a few bugfix commits but I didn't want to introduce new merge conflicts as I saw others were actively changing that code so I haven't submitted it yet, like the bug in saves which I think this PR at least partly addresses. Either way cool stuff and thanks for addressing these items! |
2 participants
Context / Goal
This PR is a follow-up to
slopus/happy-cli#107(profiles/env expansion + tmux + runtime/offline changes).The goal is to make tmux spawning correct and deterministic, align the spawn RPC contract with reality, restore reliable offline buffering, and harden logging.
Summary (what this PR does)
What slopus#107 introduced (and what we’re fixing here)
tmux correctness
-evars asKEY="escapedValue"(tmux treats quotes/backslashes literally), corrupting env values.exitCode=null(killed by signal) as success (0).TMUX_TMPDIR contract gap
Fixed by passing TMUX_TMPDIR into the tmux subprocess env and plumbing it through tmux utilities.
Offline behavior (“don’t send if disconnected”)
Fixed by removing the forced return, as Socket.IO will buffers the messages in case of deconnections and will send them on reconnect
Bun runtime support
Provider config objects (ghost overrides / leftovers?)
RPC typing drift
environmentVariablesas a narrow object with a few keys, but in practice the GUI sends arbitrary env maps.What this series fixes / improves
1) Stop unintended profile application from CLI settings
settings.activeProfileIdwhen GUI didn’t provide env vars, meaning profiles could affect sessions even when the GUI profiles feature was disabled or not used by users (in "standard" non-wizard new session modal, when wizard was disabled in settings).SpawnSessionOptions.environmentVariables.2) Persist profile identity per session (cross-device visibility)
profileId?: string | nullthrough spawn → daemon → child env (HAPPY_SESSION_PROFILE_ID) → session metadata (non-secret, used for display/debugging).3) Align CLI profile schema + env template support
${VAR}/${VAR:-default})min(1))${VAR:=default}expansion (CLI env-template parity)4) Prevent secret leakage in daemon + RPC logs
~/.happy/logs/*.5) Prevent secret leakage in
happy doctorsettings.jsonverbatim, including profile env var values.${VAR}templates visible; avoid printing legacy caches.6) Remove unwired / misleading functionality (strict cleanup)
startupBashScriptfrom the CLI profile schema (it was never executed/passed to spawn by the UI and not wired anywhere in the CLI).localEnvironmentVariablesfrom CLI settings (schema bump + migration deletes legacy key).7) Tmux: optional session-env persistence, but only for profile-defined env
-e KEY=VALUE(spawn-time).TMUX_UPDATE_ENVIRONMENTis set (truthy), we additionally persist only profile-provided env vars into the tmux session viatmux set-environment.8) Tmux: added optionnal integration tests to validate the real behavior against real tmux process
HAPPY_CLI_TMUX_INTEGRATION=1 yarn testLinks to the detailled issues in slopus/happy#107 (links go to the PR diff)
https://github.com/slopus/happy-cli/pull/107/files#diff-bf81cb7695a41b1ac6a86f3628e0fd1550eafd11acaa012b842fd1d7c8b58b77R822
https://github.com/slopus/happy-cli/pull/107/files#diff-bf81cb7695a41b1ac6a86f3628e0fd1550eafd11acaa012b842fd1d7c8b58b77R371
https://github.com/slopus/happy-cli/pull/107/files#diff-bf81cb7695a41b1ac6a86f3628e0fd1550eafd11acaa012b842fd1d7c8b58b77R383
https://github.com/slopus/happy-cli/pull/107/files#diff-bf81cb7695a41b1ac6a86f3628e0fd1550eafd11acaa012b842fd1d7c8b58b77R757
https://github.com/slopus/happy-cli/pull/107/files#diff-bf81cb7695a41b1ac6a86f3628e0fd1550eafd11acaa012b842fd1d7c8b58b77R768
https://github.com/slopus/happy-cli/pull/107/files#diff-06c3fc9035e20f951de950794a63f145b87bbdd1e7ef7c2649c44843b687f998R368
https://github.com/slopus/happy-cli/pull/107/files#diff-bf81cb7695a41b1ac6a86f3628e0fd1550eafd11acaa012b842fd1d7c8b58b77R785
https://github.com/slopus/happy-cli/pull/107/files#diff-bf81cb7695a41b1ac6a86f3628e0fd1550eafd11acaa012b842fd1d7c8b58b77R505
https://github.com/slopus/happy-cli/pull/107/files#diff-d74addbec17763255f65ef752a33e2f0355f77d42534c05a8518ba3b3fa3bfbfR153
https://github.com/slopus/happy-cli/pull/107/files#diff-bf81cb7695a41b1ac6a86f3628e0fd1550eafd11acaa012b842fd1d7c8b58b77R429
https://github.com/slopus/happy-cli/pull/107/files#diff-06c3fc9035e20f951de950794a63f145b87bbdd1e7ef7c2649c44843b687f998R306
https://github.com/slopus/happy-cli/pull/107/files#diff-d74addbec17763255f65ef752a33e2f0355f77d42534c05a8518ba3b3fa3bfbfR210
https://github.com/slopus/happy-cli/pull/107/files#diff-d74addbec17763255f65ef752a33e2f0355f77d42534c05a8518ba3b3fa3bfbfR21
https://github.com/slopus/happy-cli/pull/107/files#diff-d74addbec17763255f65ef752a33e2f0355f77d42534c05a8518ba3b3fa3bfbfR66
https://github.com/slopus/happy-cli/pull/107/files#diff-2cf782ccff90c25c92b84eb59034f21d11ee837b16a027b4a87e19519eb5be73R55
https://github.com/slopus/happy-cli/pull/107/files#diff-2cf782ccff90c25c92b84eb59034f21d11ee837b16a027b4a87e19519eb5be73R64
https://github.com/slopus/happy-cli/pull/107/files#diff-2cf782ccff90c25c92b84eb59034f21d11ee837b16a027b4a87e19519eb5be73R74
https://github.com/slopus/happy-cli/pull/107/files#diff-788c5a6eb4c0b79926b08e540d718ca3295f80256e8a88f575f7aebf05c9aedeR103
https://github.com/slopus/happy-cli/pull/107/files#diff-06c3fc9035e20f951de950794a63f145b87bbdd1e7ef7c2649c44843b687f998R391
https://github.com/slopus/happy-cli/pull/107/files#diff-f3c25005a565e61487bcb17692081309cf417703062b0806af621b1b35e76621R125
https://github.com/slopus/happy-cli/pull/107/files#diff-eb17ee6fce538711a2e94bbe7b1a749179e8494626440ad322b49b9c2685347bR197
(Note: pre-Feature/new session wizard ux improvements merged slopus/happy-cli#107 Codex path logged “Message will be lost” but still emitted; Feature/new session wizard ux improvements merged slopus/happy-cli#107 adds an actual early-return drop on the Claude path.)
https://github.com/slopus/happy-cli/pull/107/files#diff-433b730aa504dae4deceb70e389bdcf65e4f599970240f6a44ac29028e6d92d1R52
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.