Repository navigation
fix: stabilize PR107 tmux spawn, offline buffering, and bun runtime - #12
leeroybrun wants to merge 25 commits into
Conversation
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.
- 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
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.
The GUI sends a profile-derived env var map that may include provider-specific keys and tmux knobs; type it as Record<string,string>.
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.
…2026-01-12 # Conflicts: # src/api/apiSession.ts # src/utils/offlineSessionStub.ts
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughCentralizes environment handling and profile migration, expands tmux capabilities and subprocess invocation, adds Codex permission elicitation, converts offline session stub to a class, threads profileId through session spawn paths, improves logging and disconnect-send handling, and adds extensive tmux and utility tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User / CLI
participant Daemon as Daemon Process
participant Env as Env Merger
participant Tmux as Tmux Server
participant Session as Spawned Session
CLI->>Daemon: spawn-happy-session(params, profileId)
Daemon->>Env: build final env (profileEnv + sessionProfileEnv + authEnv)
Env-->>Daemon: merged env (sanitized)
Daemon->>Daemon: buildTmuxSpawnConfig(commandTokens, tmuxEnv, tmuxCommandEnv)
Daemon->>Tmux: tmux new-window -e KEY=VALUE ... -P -F ...
Tmux->>Session: start process with argv/env
Session-->>CLI: return sessionId, pid, sessionName/windowName
sequenceDiagram
participant Client as CodexMcpClient
participant Validator as Zod Validator
participant Perm as PermissionHandler
participant User as Operator
Client->>Client: receive elicitation request { params }
Client->>Validator: validate params (CodexBashElicitationParamsSchema)
alt valid
Client->>Perm: permissionHandler.handleToolCall(params)
Perm->>User: prompt for decision
User-->>Perm: allow / deny
Perm-->>Client: decision
Client-->>Caller: { decision, reason? }
else invalid
Validator-->>Client: validation error
Client-->>Caller: { decision: 'denied', reason: validation error }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
|
@gemini-code-assist @CodeRabbit please review this PR and especially against the original slopus PR 272 changes to make sure that we have caught all the issues from it and that we have not missed anything, and more importantly, that ALL our fixes and changes are in fact correct and that we do not introduce any new issue or wrong behavior |
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 significantly refines the happy-cli's interaction with tmux and its overall stability, addressing several critical issues and inconsistencies that arose from a previous major update. The primary goal is to ensure correct and deterministic tmux session management, improve the robustness of offline operations, and enhance security by preventing sensitive data leakage in logs and configurations. These changes collectively aim to provide a more reliable and predictable user experience, particularly for users leveraging tmux and profiles. 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 an excellent and comprehensive pull request that addresses a wide range of critical issues, primarily focused on tmux integration, offline behavior, and security. The fixes for environment variable injection, command argument quoting, and deterministic session selection in tmux are particularly well-implemented and crucial for correctness. Restoring offline buffering for socket sends is a great improvement for reliability. The significant effort in hardening the application by redacting sensitive information in logs and happy doctor output is highly commendable. Furthermore, the refactoring of profile schemas and subprocess spawning logic greatly improves maintainability and robustness. The addition of new unit and integration tests, especially the real tmux integration tests, provides strong confidence in these complex changes. Overall, this is a high-quality contribution that significantly enhances the stability, security, and correctness of the CLI.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 929 🏁 Script executed: Length of output: 14183
PR PR The objectives document you provided describes fixing 14 issues from slopus PR Could you confirm which PR you'd like me to review?
🏁 Script executed: Length of output: 2550 Perfect! PR 🏁 Script executed: Length of output: 226 🏁 Script executed: Length of output: 244 🏁 Script executed: Length of output: 482 🏁 Script executed: Length of output: 14055 🏁 Script executed: Length of output: 34709 🏁 Script executed: Length of output: 9304 🏁 Script executed: Length of output: 5432 🏁 Script executed: Length of output: 1577 🏁 Script executed: Length of output: 213 🏁 Script executed: Length of output: 8860 🏁 Script executed: Length of output: 9303 🏁 Script executed: Length of output: 5457 🏁 Script executed: Length of output: 3331
✅ Summary: All 14 Issues Properly AddressedCritical Tmux Fixes (Issues 1-6)Issue 1: tmux env injection corrupting values - FIXED
Issue 2: $TMUX parsing - CORRECT
Issue 3: Empty session selection nondeterministic - FIXED
Issue 4: Command string via args.join(' ') - FIXED
Issue 5: exitCode=null treated as success - FIXED
Issue 6: TMUX_TMPDIR not passed to tmux client - FIXED
Profile & Security Fixes (Issues 7-10)Issue 7: Daemon fallback to CLI activeProfileId - FIXED
Issue 8: localEnvironmentVariables cache (secret leak) - FIXED
Issue 9: Profile schema too strict - FIXED
Issue 10: expandEnvVars logs secret values - FIXED
Runtime & RPC Fixes (Issues 11-12)Issue 11: Bun runtime selection incoherent - FIXED
Issue 12: SpawnSessionOptions typing too narrow - FIXED
Offline Mode Fixes (Issues 13-14)Issue 13: Offline message drop (early return) - FIXED
Issue 14: Offline stub type assertion - IMPROVED
🎯 Additional Improvements Found
|
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive pull request that addresses numerous critical issues, particularly around tmux integration, security, and runtime robustness. The fixes for environment variable handling, command quoting, and deterministic session selection in tmux are significant improvements. I'm also impressed with the security enhancements to prevent secret leakage in logs and the doctor command, as well as the restoration of offline message buffering. The addition of real tmux integration tests is a fantastic step towards ensuring long-term stability. I have a few minor suggestions to improve code conciseness and readability in a couple of spots, but overall, this is a very strong contribution.
|
@coderabbitai @gemini-code-assist please check the new update that has been pushed to the branch and advise if there are any remaining/new issues or if all the issues that we reported are fixed now. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12695 🏁 Script executed: Length of output: 4262 🏁 Script executed: Length of output: 6293
✅ Latest Changes Look Good - No New IssuesKey Changes in Latest Commit:1. TMUX_UPDATE_ENVIRONMENT Feature Removed ✅
2. Enhanced Disconnected Send Logging ✅
3. Doctor Command Cleanup ✅
4. Code Quality Improvements ✅
🎯 Final Status: All Issues ResolvedComparing against the original 14 issues from PR
SummaryThe PR is in excellent shape. The latest updates represent thoughtful refinements based on:
Recommendation: The PR is ready to merge. All reported issues are fixed, no new problems introduced, and the latest changes improve security and code quality. 🧠 Learnings used |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that delivers a comprehensive set of fixes and improvements, primarily focused on tmux integration, offline buffering, and security hardening. The changes address numerous significant issues, including potential command injection vulnerabilities, secret leakage in logs, and incorrect environment variable handling. The refactoring is well-executed, making the code more modular and testable. The addition of both unit and integration tests for the tmux functionality is particularly valuable and demonstrates a commitment to quality and correctness. Overall, this is a high-quality contribution that significantly improves the robustness and security of the application. I have one minor suggestion for consistency.
| host: os.hostname(), | ||
| version: packageJson.version, | ||
| os: os.platform(), | ||
| profileId, |
There was a problem hiding this comment.
For consistency with createSessionMetadata.ts and to avoid including a profileId key with an undefined value, it's better to conditionally add the property to the metadata object. This ensures that the key is only present when profileIdEnv is defined.
| profileId, | |
| ...(profileIdEnv !== undefined ? { profileId } : {}), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/daemon/run.ts (1)
312-322: Temporary directory for Codex auth not cleaned up.
tmp.dirSync()returns a directory object with aremoveCallback()method that must be explicitly called to delete the directory. Currently, thecodexHomeDirobject goes out of scope after line 318, andremoveCallback()is never invoked. This leaves temporary directories on disk when sessions fail or terminate, causing accumulation over time.Consider storing a reference to
codexHomeDir.removeCallbackand calling it during daemon shutdown in thecleanupAndShutdown()function, or usetmp.setGracefulCleanup()to automatically track and clean up temporary directories on process exit.
🤖 Fix all issues with AI agents
In `@src/utils/tmux.ts`:
- Around line 990-1002: Validation trims sessionName into trimmedSessionName but
later constructs/returns the session identifier using the original sessionName
(potentially with whitespace); update all uses that build the tmux
identifier/result (including where TmuxUtilities is instantiated, the result
identifier and any calls that reference sessionName such as in
TmuxUtilities(...) or value returned to callers) to use trimmedSessionName
instead of sessionName and ensure windowName/command construction still uses the
validated trimmed value (refer to TmuxUtilities, TmuxSessionIdentifierError,
trimmedSessionName, sessionName, windowName, cmd).
🧹 Nitpick comments (4)
src/utils/tmux.real.integration.test.ts (1)
120-164: Consider usingos.tmpdir()for cross-platform compatibility.The hardcoded
/tmp/prefix works on Unix-like systems but could fail on Windows or systems with non-standard temp directories. Usingos.tmpdir()would improve portability.Suggested improvement
import { tmpdir } from 'node:os'; // ... const dir = mkdtempSync(join(tmpdir(), 'happy-cli-tmux-it-'));The test logic itself is solid: it verifies the pane PID from
spawnInTmuxmatches the ground truth fromtmux list-panes, and validates the TMUX environment variable format.src/ui/doctor.ts (1)
20-28: Consider extending template detection to include default value syntax.The regex
/^\$\{[A-Z_][A-Z0-9_]*\}$/matches simple templates like${PATH}but not the${VAR:=default}or${VAR:-default}formats that the PR objectives mention as newly supported. Templates with default values would be masked as<N chars>rather than displayed.🔧 Suggested regex to include default value syntax
- if (/^\$\{[A-Z_][A-Z0-9_]*\}$/.test(value)) return value; + // Match ${VAR}, ${VAR:=default}, ${VAR:-default} patterns + if (/^\$\{[A-Z_][A-Z0-9_]*(:[=-][^}]*)?\}$/.test(value)) return value;src/codex/codexMcpClient.ts (1)
57-65: Consider exporting the schema and type for testing.
CodexBashElicitationParamsSchemaandCodexBashElicitationParamsare not exported but may be useful for unit tests that need to construct valid elicitation parameters or validate against the expected schema.🔧 Export schema and type if needed for tests
-const CodexBashElicitationParamsSchema = z +export const CodexBashElicitationParamsSchema = z .object({ codex_call_id: z.string(), codex_command: z.string(), codex_cwd: z.string().optional(), }) .passthrough(); -type CodexBashElicitationParams = z.infer<typeof CodexBashElicitationParamsSchema>; +export type CodexBashElicitationParams = z.infer<typeof CodexBashElicitationParamsSchema>;src/persistence.profileSchema.test.ts (1)
1-2: Add a short file header comment describing test intent.Keeps file-level responsibility docs consistent.
As per coding guidelines, please keep file-level responsibility comments.📝 Suggested header comment
+/** + * Tests migration of legacy provider configs into environmentVariables. + */ import { describe, expect, it } from 'vitest'; import { AIBackendProfileSchema } from './persistence';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/api/api.test.tssrc/api/apiMachine.tssrc/api/apiSession.test.tssrc/api/apiSession.tssrc/claude/runClaude.tssrc/claude/utils/sessionScanner.test.tssrc/codex/codexMcpClient.tssrc/daemon/run.tmuxEnv.test.tssrc/daemon/run.tmuxSpawn.test.tssrc/daemon/run.tssrc/modules/common/registerCommonHandlers.tssrc/persistence.profileSchema.test.tssrc/persistence.tssrc/ui/doctor.test.tssrc/ui/doctor.tssrc/ui/logger.test.tssrc/utils/createSessionMetadata.tssrc/utils/offlineSessionStub.test.tssrc/utils/offlineSessionStub.tssrc/utils/spawnHappyCLI.invocation.test.tssrc/utils/tmux.commandEnv.test.tssrc/utils/tmux.real.integration.test.tssrc/utils/tmux.socketPath.test.tssrc/utils/tmux.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/daemon/run.tmuxSpawn.test.ts
- src/utils/offlineSessionStub.ts
- src/claude/utils/sessionScanner.test.ts
- src/utils/tmux.socketPath.test.ts
- src/utils/offlineSessionStub.test.ts
- src/utils/spawnHappyCLI.invocation.test.ts
- src/modules/common/registerCommonHandlers.ts
- src/api/apiSession.ts
- src/utils/createSessionMetadata.ts
- src/utils/tmux.commandEnv.test.ts
- src/ui/doctor.test.ts
- src/api/apiSession.test.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use descriptive variable and function names that clearly convey intent
Write clear, concise comments explaining the 'why' behind complex logic
Keep functions small and focused on a single responsibility
Use consistent code formatting and adhere to the project's style guide
Files:
src/api/api.test.tssrc/persistence.profileSchema.test.tssrc/api/apiMachine.tssrc/ui/logger.test.tssrc/codex/codexMcpClient.tssrc/persistence.tssrc/ui/doctor.tssrc/claude/runClaude.tssrc/daemon/run.tssrc/daemon/run.tmuxEnv.test.tssrc/utils/tmux.real.integration.test.tssrc/utils/tmux.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: No untyped code - use strict typing in TypeScript with explicit parameter and return types
Use comprehensive JSDoc comments - each file includes header comments explaining responsibilities
ALL imports must be at the top of the file - NEVER import modules mid-code
Use named exports preferred, with occasional default exports for main functions
Avoid creating stupid small functions, getters, or setters - minimize unnecessary function decomposition
Avoid excessive use ofifstatements - especially if you can avoid control flow changes with a better design
Files:
src/api/api.test.tssrc/persistence.profileSchema.test.tssrc/api/apiMachine.tssrc/ui/logger.test.tssrc/codex/codexMcpClient.tssrc/persistence.tssrc/ui/doctor.tssrc/claude/runClaude.tssrc/daemon/run.tssrc/daemon/run.tmuxEnv.test.tssrc/utils/tmux.real.integration.test.tssrc/utils/tmux.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Use@/alias for src imports, e.g.,import { logger } from '@/ui/logger'
Use graceful error handling with proper error messages, try-catch blocks with specific error logging, and abort controllers for cancellable operations
All debugging must be through file logs to avoid disturbing Claude sessions - console output only for user-facing messages
Handle large JSON objects with truncation in logs to avoid excessive output
Minimize use of classes - prefer functional approaches in TypeScript
Files:
src/api/api.test.tssrc/persistence.profileSchema.test.tssrc/api/apiMachine.tssrc/ui/logger.test.tssrc/codex/codexMcpClient.tssrc/persistence.tssrc/ui/doctor.tssrc/claude/runClaude.tssrc/daemon/run.tssrc/daemon/run.tmuxEnv.test.tssrc/utils/tmux.real.integration.test.tssrc/utils/tmux.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
.tsfile extensions for TypeScript files
Files:
src/api/api.test.tssrc/persistence.profileSchema.test.tssrc/api/apiMachine.tssrc/ui/logger.test.tssrc/codex/codexMcpClient.tssrc/persistence.tssrc/ui/doctor.tssrc/claude/runClaude.tssrc/daemon/run.tssrc/daemon/run.tmuxEnv.test.tssrc/utils/tmux.real.integration.test.tssrc/utils/tmux.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Write unit tests using Vitest with no mocking - tests should make real API calls
Colocate test files with source files using.test.tsnaming convention
Use descriptive test names and proper async handling in tests
Files:
src/api/api.test.tssrc/persistence.profileSchema.test.tssrc/ui/logger.test.tssrc/daemon/run.tmuxEnv.test.tssrc/utils/tmux.real.integration.test.ts
src/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/api/**/*.{ts,tsx}: Implement end-to-end encryption for all communications using TweetNaCl
Use Socket.IO for real-time messaging in API sessions
Implement optimistic concurrency control for state updates in sessions
Implement session isolation through unique session IDs
Files:
src/api/api.test.tssrc/api/apiMachine.ts
src/{auth,persistence}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Store private keys in
~/.handy/access.keywith restricted permissions
Files:
src/persistence.ts
src/daemon/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/daemon/CLAUDE.md)
When daemon receives 'daemon start' command in src/index.ts, spawn a detached process via spawnHappyCLI(['daemon', 'start-sync'], { detached: true }) that calls startDaemon() from src/daemon/run.ts
Files:
src/daemon/run.tssrc/daemon/run.tmuxEnv.test.ts
🧠 Learnings (35)
📓 Common learnings
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Track spawned Happy processes by storing their PID and happySessionId in daemon.state.json for cleanup purposes; include process information for both daemon-spawned and user-spawned sessions
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to **/*.test.{ts,tsx} : Use descriptive test names and proper async handling in tests
Applied to files:
src/api/api.test.tssrc/ui/logger.test.tssrc/utils/tmux.real.integration.test.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to **/*.test.{ts,tsx} : Write unit tests using Vitest with no mocking - tests should make real API calls
Applied to files:
src/api/api.test.tssrc/persistence.profileSchema.test.tssrc/ui/logger.test.tssrc/daemon/run.tmuxEnv.test.tssrc/utils/tmux.real.integration.test.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/**/*.{ts,tsx} : Use graceful error handling with proper error messages, try-catch blocks with specific error logging, and abort controllers for cancellable operations
Applied to files:
src/api/api.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement HTTP endpoint /spawn-session that creates new sessions (used by integration tests) via daemon HTTP server
Applied to files:
src/api/api.test.tssrc/api/apiMachine.tssrc/daemon/run.tssrc/daemon/run.tmuxEnv.test.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/**/*.{ts,tsx} : Implement session isolation through unique session IDs
Applied to files:
src/api/api.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When daemon receives RPC 'spawn-happy-session' from backend, create directory if needed, spawn detached Happy process with flags '--happy-starting-mode remote --started-by daemon', add to pidToTrackedSession map, and set up 10-second awaiter for session webhook
Applied to files:
src/api/apiMachine.tssrc/daemon/run.tssrc/utils/tmux.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Track spawned Happy processes by storing their PID and happySessionId in daemon.state.json for cleanup purposes; include process information for both daemon-spawned and user-spawned sessions
Applied to files:
src/api/apiMachine.tssrc/claude/runClaude.tssrc/daemon/run.tssrc/daemon/run.tmuxEnv.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement RPC method naming pattern as 'machineId:rpc-method-name' for machine-scoped RPC calls received from server
Applied to files:
src/api/apiMachine.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Register RPC handlers for 'spawn-happy-session', 'stop-session', and 'requestShutdown' on the daemon's HTTP server and WebSocket connection
Applied to files:
src/api/apiMachine.tssrc/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement machine metadata structure containing host, platform, happyCliVersion, homeDir, and happyHomeDir as static machine information that rarely changes
Applied to files:
src/api/apiMachine.tssrc/claude/runClaude.tssrc/daemon/run.tssrc/daemon/run.tmuxEnv.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/src/index.ts : In CLI startup phase, check if machine ID exists in settings; if not, create ID locally only without creating machine on server (daemon's responsibility on startup)
Applied to files:
src/api/apiMachine.tssrc/claude/runClaude.tssrc/daemon/run.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/apiSession.{ts,tsx} : Register RPC handlers for remote procedure calls in session clients
Applied to files:
src/api/apiMachine.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/**/*.{ts,tsx} : Use `@/` alias for src imports, e.g., `import { logger } from '@/ui/logger'`
Applied to files:
src/ui/logger.test.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/**/*.{ts,tsx} : All debugging must be through file logs to avoid disturbing Claude sessions - console output only for user-facing messages
Applied to files:
src/ui/logger.test.tssrc/claude/runClaude.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/**/*.{ts,tsx} : Handle large JSON objects with truncation in logs to avoid excessive output
Applied to files:
src/ui/logger.test.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to **/*.test.{ts,tsx} : Colocate test files with source files using `.test.ts` naming convention
Applied to files:
src/ui/logger.test.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/types.{ts,tsx} : Use Zod schemas for type-safe API communication in types files
Applied to files:
src/persistence.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : In 'happy doctor' command, use 'ps aux | grep' to find all Happy processes and categorize by command args: daemon, daemon-spawned, user-session, doctor; match production patterns 'happy.mjs', 'happy-coder', 'dist/index.mjs' and development pattern 'tsx.*src/index.ts'
Applied to files:
src/ui/doctor.tssrc/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : Implement 'happy doctor clean' command that calls findRunawayHappyProcesses() to filter likely orphans, then killRunawayHappyProcesses() to send SIGTERM, wait 1 second, and send SIGKILL if still alive
Applied to files:
src/ui/doctor.tssrc/daemon/run.tssrc/utils/tmux.real.integration.test.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/claude/watcher.{ts,tsx} : Implement file system watcher for Claude session files in interactive mode
Applied to files:
src/claude/runClaude.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Ensure caffeinate process is started by the daemon (not individual sessions) and tracked in daemon.state.json to prevent it from becoming a runaway process
Applied to files:
src/claude/runClaude.tssrc/daemon/run.tssrc/daemon/run.tmuxEnv.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When version mismatch is detected during heartbeat, spawn a new daemon via spawnHappyCLI(['daemon', 'start']) and wait to be killed by the new daemon instance
Applied to files:
src/daemon/run.tssrc/daemon/run.tmuxEnv.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/*.{ts,tsx} : When daemon receives 'daemon start' command in src/index.ts, spawn a detached process via spawnHappyCLI(['daemon', 'start-sync'], { detached: true }) that calls startDaemon() from src/daemon/run.ts
Applied to files:
src/daemon/run.tssrc/utils/tmux.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement a heartbeat loop in the daemon that runs every 60 seconds (or via HAPPY_DAEMON_HEARTBEAT_INTERVAL) to check for version updates and prune dead sessions
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Emit 'machine-update-metadata' WebSocket events when machine metadata changes (rare updates like version changes), including expectedVersion for version mismatch handling
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Perform authentication via authAndSetupMachineIfNeeded() during daemon startup to ensure credentials exist before establishing backend connections
Applied to files:
src/daemon/run.tssrc/daemon/run.tmuxEnv.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : During daemon startup, perform version check by reading daemon.state.json and comparing startedWithCliVersion with configuration.currentCliVersion; if version mismatch detected, call stopDaemon() to kill old daemon before proceeding
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Write daemon state including PID, HTTP port, version, and start time to daemon.state.json upon successful startup; maintain 'state' and 'stateReason' fields to explain daemon status and preserve file on exit/stop instead of hard deleting
Applied to files:
src/daemon/run.tssrc/daemon/run.tmuxEnv.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : On daemon startup, POST to /v1/machines REST endpoint with encrypted metadata and daemonState, sending machineId, and receive metadataVersion and daemonStateVersion from server response
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement daemon state structure containing status ('running' | 'shutting-down' | 'offline'), pid, httpPort, startedAt, shutdownRequestedAt, and shutdownSource as dynamic daemon runtime state
Applied to files:
src/daemon/run.tssrc/daemon/run.tmuxEnv.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement stopSession() to find session by happySessionId, send SIGTERM to process, and remove from tracking map on exit event
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement notifyDaemonSessionStarted() webhook endpoint that receives POST requests to /session-started and creates TrackedSession entries with startedBy identifier
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement cleanupAndShutdown() to clear heartbeat interval, update daemon state to 'shutting-down' on backend, disconnect WebSocket, stop HTTP server, release lock file, and write final daemon state before exit
Applied to files:
src/daemon/run.tmuxEnv.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Use acquireDaemonLock() to create an exclusive lock file with O_EXCL flag to prevent multiple daemon instances from running simultaneously
Applied to files:
src/daemon/run.tmuxEnv.test.ts
🧬 Code graph analysis (9)
src/api/api.test.ts (1)
src/utils/serverConnectionErrors.ts (1)
connectionState(338-338)
src/persistence.profileSchema.test.ts (1)
src/persistence.ts (1)
AIBackendProfileSchema(101-135)
src/api/apiMachine.ts (1)
src/ui/logger.ts (1)
logger(233-233)
src/ui/logger.test.ts (1)
src/ui/logger.ts (1)
logger(233-233)
src/codex/codexMcpClient.ts (1)
src/codex/utils/permissionHandler.ts (1)
CodexPermissionHandler(22-58)
src/ui/doctor.ts (1)
src/persistence.ts (1)
readSettings(263-312)
src/claude/runClaude.ts (2)
src/api/types.ts (1)
Metadata(302-335)src/claude/claudeLocal.ts (1)
claudeLocal(17-328)
src/daemon/run.ts (3)
src/utils/spawnHappyCLI.ts (1)
buildHappyCliSubprocessInvocation(65-87)src/utils/tmux.ts (1)
TmuxUtilities(374-956)src/daemon/types.ts (1)
TrackedSession(11-22)
src/utils/tmux.real.integration.test.ts (1)
src/utils/tmux.ts (1)
TmuxUtilities(374-956)
🔇 Additional comments (53)
src/utils/tmux.real.integration.test.ts (9)
1-14: LGTM!Clear header documentation explaining the opt-in nature and isolation guarantees. Imports are well-organized and use the
@/alias correctly per coding guidelines.
16-23: LGTM!Clean gating logic that checks both the environment variable and tmux availability. This ensures tests only run when explicitly enabled and the system has tmux installed.
25-38: LGTM!Well-implemented polling mechanism with proper timeout handling and clear error messages.
40-65: LGTM!Clever use of a CommonJS dump script to capture argv and env from inside the tmux pane. The keepAlive pattern using
setTimeoutis an effective way to keep the Node process alive for inspection.
67-88: LGTM!Proper save/restore pattern for environment variables with correct handling of
undefinedvs defined-but-empty values. The finally block ensures cleanup even on test failures.
90-118: LGTM!Well-structured helper that properly isolates tmux invocations from the user's environment. Deleting TMUX, TMUX_PANE, and TMUX_TMPDIR before invoking tmux ensures tests only interact with explicitly specified servers.
166-200: LGTM!Excellent regression test for PR107 quoting/escaping issues. Testing special characters like
$,",\, and backtick ensures environment values are passed literally without shell interpretation.
202-237: Strong security regression test.This test effectively validates that command arguments are properly quoted to prevent shell injection. The sentinel file check (line 232) confirms that the
$(touch ...)payload is treated as a literal string, not executed by the shell.
239-284: LGTM!Comprehensive test for TMUX_TMPDIR isolation. The test correctly:
- Uses
withCleanTmuxClientEnvto prevent interference from the test runner's environment- Verifies session invisibility on the default server (line 268)
- Confirms session visibility on the isolated server with correct TMUX_TMPDIR (line 273)
Good comment on line 241-242 about unix domain socket path length limits.
src/ui/logger.test.ts (4)
11-16: LGTM! Proper test isolation with module reset.The
beforeEachcorrectly:
- Creates a fresh temp directory for each test
- Sets
HAPPY_HOME_DIRto isolate file writes- Deletes
DEBUGto establish a known baseline- Calls
vi.resetModules()to ensure fresh module stateThis setup ensures tests don't interfere with each other.
18-25: Clean environment restoration logic.The
afterEachproperly distinguishes between an originally-undefined env var (which should be deleted) and an originally-defined one (which should be restored). This prevents test pollution.
27-33: LGTM! Correctly verifies DEBUG-gated behavior.This test properly validates that
debugLargeJsonperforms no file I/O when DEBUG is unset, aligning with the production behavior described in the AI summary.
35-44: Remove timing concern—no issue exists.The test is correct as written. The
Loggerconstructor does not capture the DEBUG state; it only setslogFilePath. ThedebugLargeJsonmethod checksprocess.env.DEBUGat call time (when the method executes), not at construction time. Settingprocess.env.DEBUG = '1'after the import but before callinglogger.debugLargeJson()is valid and works as intended.src/utils/tmux.ts (12)
26-30: LGTM! Correct handling of signal-terminated processes.Converting
nullexit codes to1preserves failure semantics when processes are terminated by signals, preventing them from being incorrectly treated as successful exits.
32-39: LGTM! POSIX-safe shell escaping.The single-quote escaping approach (
' -> '\'') is the standard POSIX-safe method for shell argument quoting. This correctly handles arbitrary strings including those with special characters.
94-101: LGTM! Interface reflects actual TMUX env var structure.The updated
TmuxEnvironmentinterface now correctly represents the three components of the TMUX environment variable: socket path, server PID, and pane identifier.
380-387: LGTM! Constructor extended for environment and socket path wiring.The addition of
tmuxCommandEnvandtmuxSocketPathparameters enables proper environment propagation to tmux client invocations, addressing the TMUX_TMPDIR wiring fix mentioned in PR objectives.
398-416: LGTM! Robust TMUX env var parsing.The parsing now correctly handles all three components with proper validation (numeric check for server PID) and falls back to
TMUX_PANEenvironment variable for pane identification.
440-443: LGTM! Socket path resolution with proper precedence.The fallback chain (
socketPathparameter →this.tmuxSocketPath→ none) correctly prioritizes per-call overrides over instance defaults.
499-511: LGTM! Environment merging with correct precedence.The merge order (
process.env→tmuxCommandEnv→options.env) ensures instance-level environment is applied, with per-call options taking final precedence.
524-529: LGTM! Exit code normalization applied.Using
normalizeExitCode(code)ensures signal-terminated processes are treated as failures, fixing the regression mentioned in PR objectives.
770-805: LGTM! Deterministic empty-session selection logic.When
sessionNameis empty string, the code queries available sessions and sorts by attached status then by last-attached timestamp, ensuring deterministic selection of the most relevant session.
813-853: LGTM! Correct command building with POSIX quoting and literal -e values.The command is built using
buildPosixShellCommand(args)for shell safety, and environment variables are passed via-e KEY=VALUEwithout shell escaping (as tmux processes them literally). The explicit-t sessionNameplacement ensures correct option ordering.
876-882: LGTM! Return payload expanded with sessionName and windowName.The expanded return type allows callers to know the actual session name used (important when input was empty string for "most recent" selection).
153-158: The regex correctly allows spaces in session names without additional concerns.Tmux supports spaces in session names and the code safely handles them. The implementation uses
spawn()withshell: false, which passes arguments directly to the tmux binary without shell interpretation. This means no additional quoting or escaping is required—the session name (built as a single command-line argument via the target specification) is passed safely to tmux. The regex validation is appropriate and complete.src/daemon/run.ts (8)
36-45: LGTM! Concise implementation using Object.fromEntries.This implementation correctly filters out
undefinedvalues fromdaemonEnvand merges withextraEnv, withextraEnvtaking precedence. The approach follows the past review suggestion.
47-82: LGTM! Well-structured spawn config builder.The function cleanly encapsulates:
- CLI subprocess invocation building (respecting Bun/Node runtime selection)
- Environment merging for tmux window
- TMUX_TMPDIR propagation for tmux client commands
This separation enables easier testing and consistent behavior between tmux and non-tmux paths.
236-251: LGTM! Sensitive data properly sanitized in logs.The logging now only captures non-sensitive metadata (directory, sessionId, agent, key counts) rather than raw environment values, preventing accidental token leakage.
324-337: LGTM! Explicit profile env handling without implicit fallback.The removal of CLI-local profile fallback ensures GUI-spawned sessions respect GUI opt-in gating, aligning with the PR objective to only apply profile env when explicitly provided.
339-348: LGTM! Session identity propagation with correct empty-string handling.
HAPPY_SESSION_PROFILE_IDis set whenprofileId !== undefined, correctly preserving empty string as "no profile" signal. The merge order (profileEnv→sessionProfileEnv→authEnv) ensures auth tokens cannot be overridden.
407-421: LGTM! Tmux spawn config properly wired.The new
buildTmuxSpawnConfighelper is used correctly, andTmuxUtilitiesis instantiated withtmuxCommandEnvto ensureTMUX_TMPDIRis passed to tmux client invocations.
431-443: LGTM! Proper tmux session name resolution for messaging.When
spawnInTmuxresolves the actual session name (important for empty-string input), the message correctly reflects the resolved session name for user-facing instructions.
507-516: Sessionid comment is accurate. No--resumefunctionality exists in the codebase. ThesessionIdparameter is accepted but never used for resumption logic, only logged. The comment correctly indicates it's reserved for future implementation.src/daemon/run.tmuxEnv.test.ts (1)
1-16: LGTM! Test covers key merge behaviors.The test validates:
- Base env values are preserved (
PATH)- Extra env overrides base env (
HOME)- Extra env additions are included (
CUSTOM)- Undefined values are filtered out (
UNDEFINED)This aligns with the
buildTmuxWindowEnvimplementation. As per coding guidelines, tests use Vitest with no mocking.src/ui/doctor.ts (2)
32-57: LGTM!The redaction logic correctly handles both the legacy
localEnvironmentVariablesremoval and per-profile environment variable masking. The deep clone approach safely isolates the display copy from the original settings.
162-162: LGTM!Properly redacts sensitive settings before display, preventing secret leakage in doctor output.
src/api/apiMachine.ts (1)
105-128: LGTM!The enhanced logging provides valuable debugging information while maintaining security:
- Uses
hasTokeninstead of logging the actual token- Samples environment variable keys with truncation indicator
- Correctly propagates
profileIdthrough the spawn chainsrc/api/api.test.ts (3)
179-198: LGTM!The try/finally pattern ensures cleanup happens regardless of test outcome, improving test isolation and reliability.
205-224: LGTM!Consistent application of the try/finally cleanup pattern.
229-246: LGTM!Proper cleanup flow for the error re-throw test case.
src/claude/runClaude.ts (2)
86-88: LGTM!The profileId extraction logic correctly handles the three cases: undefined environment variable, empty/whitespace value (→ null), and valid value (→ trimmed string). This follows the pattern suggested in past reviews.
135-157: LGTM!The AbortController pattern for the offline flow is well-implemented:
- Signal handlers are properly registered and removed in the finally block
- Reconnection is cancelled and caffeinate is stopped on cleanup
- Prevents signal handler accumulation across multiple invocations
src/codex/codexMcpClient.ts (3)
24-29: LGTM!Returning
'mcp-server'when the version format is unexpected is a sensible default that avoids falsely treating codex as "not installed" when it's merely running an unexpected version format.
67-120: LGTM!The factory function is well-designed:
- Supports both direct handler instances and lazy provider functions
- Validates inputs with zod before invoking the permission handler
- Has proper error handling with informative denial reasons
- The denial-by-default when no handler is set is a safe security posture
200-200: LGTM!Using a lambda
() => this.permissionHandlerallows the handler to be set after registration viasetPermissionHandler(), which is the expected usage pattern.src/persistence.profileSchema.test.ts (2)
4-16: Good coverage of legacy config migration.The assertions validate env-var migration and removal of legacy fields.
18-31: Good guard against legacy overrides.This ensures explicit environment variables stay authoritative.
src/persistence.ts (6)
19-50: Merge behavior preserves explicit env vars.Keeping existing entries and seeing additions as defaults matches the intended precedence.
52-79: Legacy profile normalization is clean and focused.Mapping legacy provider fields into environmentVariables and stripping old keys keeps consumers consistent.
100-135: Schema normalization before validation looks solid.The normalized shape aligns profile handling with the unified env-var model.
156-157: TMUX_TMPDIR passthrough is handled correctly.Including explicit empty strings matches the intended tmux default behavior.
181-182: Schema version bump is clear and scoped.The local-settings note clarifies the version context.
236-242: v2→v3 cleanup is straightforward.Removing the deprecated localEnvironmentVariables aligns with the new schema.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
leeroybrun
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/tmux.real.integration.test.ts`:
- Line 150: The test uses a tight 2_000 ms hardcoded timeout in waitForFile
calls which causes flakiness; create a shared constant (e.g., const
FILE_WAIT_TIMEOUT_MS = 5_000 or derive from the suite timeout) and replace the
literal 2_000 in waitForFile(outFile, 2_000) at the occurrences inside
src/utils/tmux.real.integration.test.ts (including lines where waitForFile is
called around the reported locations) with waitForFile(outFile,
FILE_WAIT_TIMEOUT_MS), and update any other waitForFile invocations (e.g., the
ones noted at the other call sites) to use the same constant so the timeout is
consistent and easier to tweak for CI.
🧹 Nitpick comments (2)
src/daemon/run.ts (1)
16-16: Unused import:readSettings
readSettingsis imported but not used anywhere in this file.🧹 Suggested fix
-import { writeDaemonState, DaemonLocallyPersistedState, readDaemonState, acquireDaemonLock, releaseDaemonLock, readSettings } from '@/persistence'; +import { writeDaemonState, DaemonLocallyPersistedState, readDaemonState, acquireDaemonLock, releaseDaemonLock } from '@/persistence';src/utils/tmux.real.integration.test.ts (1)
151-152: Avoidanywhen parsing dump payloads.A small
DumpPayloadtype + helper keeps the tests strictly typed and removes repetition. As per coding guidelines.♻️ Suggested typing helper
+type DumpPayload = { + argv: string[]; + env: { + FOO?: string; + BAR?: string; + TMUX?: string; + TMUX_PANE?: string; + TMUX_TMPDIR?: string; + }; +}; + +function readDumpPayload(path: string): DumpPayload { + return JSON.parse(readFileSync(path, 'utf8')) as DumpPayload; +}- const payload = JSON.parse(readFileSync(outFile, 'utf8')) as any; + const payload = readDumpPayload(outFile);- const payload = JSON.parse(readFileSync(outFile, 'utf8')) as any; + const payload = readDumpPayload(outFile);- const payload = JSON.parse(readFileSync(outFile, 'utf8')) as any; + const payload = readDumpPayload(outFile);- const payload = JSON.parse(readFileSync(outFile, 'utf8')) as any; + const payload = readDumpPayload(outFile);Also applies to: 193-194, 229-230, 277-278
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/daemon/run.tssrc/ui/doctor.test.tssrc/ui/doctor.tssrc/utils/tmux.real.integration.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use descriptive variable and function names that clearly convey intent
Write clear, concise comments explaining the 'why' behind complex logic
Keep functions small and focused on a single responsibility
Use consistent code formatting and adhere to the project's style guide
Files:
src/ui/doctor.test.tssrc/daemon/run.tssrc/ui/doctor.tssrc/utils/tmux.real.integration.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: No untyped code - use strict typing in TypeScript with explicit parameter and return types
Use comprehensive JSDoc comments - each file includes header comments explaining responsibilities
ALL imports must be at the top of the file - NEVER import modules mid-code
Use named exports preferred, with occasional default exports for main functions
Avoid creating stupid small functions, getters, or setters - minimize unnecessary function decomposition
Avoid excessive use ofifstatements - especially if you can avoid control flow changes with a better design
Files:
src/ui/doctor.test.tssrc/daemon/run.tssrc/ui/doctor.tssrc/utils/tmux.real.integration.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Use@/alias for src imports, e.g.,import { logger } from '@/ui/logger'
Use graceful error handling with proper error messages, try-catch blocks with specific error logging, and abort controllers for cancellable operations
All debugging must be through file logs to avoid disturbing Claude sessions - console output only for user-facing messages
Handle large JSON objects with truncation in logs to avoid excessive output
Minimize use of classes - prefer functional approaches in TypeScript
Files:
src/ui/doctor.test.tssrc/daemon/run.tssrc/ui/doctor.tssrc/utils/tmux.real.integration.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
.tsfile extensions for TypeScript files
Files:
src/ui/doctor.test.tssrc/daemon/run.tssrc/ui/doctor.tssrc/utils/tmux.real.integration.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Write unit tests using Vitest with no mocking - tests should make real API calls
Colocate test files with source files using.test.tsnaming convention
Use descriptive test names and proper async handling in tests
Files:
src/ui/doctor.test.tssrc/utils/tmux.real.integration.test.ts
src/daemon/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/daemon/CLAUDE.md)
When daemon receives 'daemon start' command in src/index.ts, spawn a detached process via spawnHappyCLI(['daemon', 'start-sync'], { detached: true }) that calls startDaemon() from src/daemon/run.ts
Files:
src/daemon/run.ts
🧠 Learnings (31)
📓 Common learnings
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Track spawned Happy processes by storing their PID and happySessionId in daemon.state.json for cleanup purposes; include process information for both daemon-spawned and user-spawned sessions
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When daemon receives RPC 'spawn-happy-session' from backend, create directory if needed, spawn detached Happy process with flags '--happy-starting-mode remote --started-by daemon', add to pidToTrackedSession map, and set up 10-second awaiter for session webhook
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : In 'happy doctor' command, use 'ps aux | grep' to find all Happy processes and categorize by command args: daemon, daemon-spawned, user-session, doctor; match production patterns 'happy.mjs', 'happy-coder', 'dist/index.mjs' and development pattern 'tsx.*src/index.ts'
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : Implement 'happy doctor clean' command that calls findRunawayHappyProcesses() to filter likely orphans, then killRunawayHappyProcesses() to send SIGTERM, wait 1 second, and send SIGKILL if still alive
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : In 'happy doctor' command, use 'ps aux | grep' to find all Happy processes and categorize by command args: daemon, daemon-spawned, user-session, doctor; match production patterns 'happy.mjs', 'happy-coder', 'dist/index.mjs' and development pattern 'tsx.*src/index.ts'
Applied to files:
src/ui/doctor.test.tssrc/daemon/run.tssrc/ui/doctor.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to **/*.test.{ts,tsx} : Write unit tests using Vitest with no mocking - tests should make real API calls
Applied to files:
src/ui/doctor.test.tssrc/utils/tmux.real.integration.test.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to **/*.test.{ts,tsx} : Use descriptive test names and proper async handling in tests
Applied to files:
src/ui/doctor.test.tssrc/utils/tmux.real.integration.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Track spawned Happy processes by storing their PID and happySessionId in daemon.state.json for cleanup purposes; include process information for both daemon-spawned and user-spawned sessions
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When version mismatch is detected during heartbeat, spawn a new daemon via spawnHappyCLI(['daemon', 'start']) and wait to be killed by the new daemon instance
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : When daemon receives RPC 'spawn-happy-session' from backend, create directory if needed, spawn detached Happy process with flags '--happy-starting-mode remote --started-by daemon', add to pidToTrackedSession map, and set up 10-second awaiter for session webhook
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/*.{ts,tsx} : When daemon receives 'daemon start' command in src/index.ts, spawn a detached process via spawnHappyCLI(['daemon', 'start-sync'], { detached: true }) that calls startDaemon() from src/daemon/run.ts
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement machine metadata structure containing host, platform, happyCliVersion, homeDir, and happyHomeDir as static machine information that rarely changes
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Emit 'machine-update-metadata' WebSocket events when machine metadata changes (rare updates like version changes), including expectedVersion for version mismatch handling
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement a heartbeat loop in the daemon that runs every 60 seconds (or via HAPPY_DAEMON_HEARTBEAT_INTERVAL) to check for version updates and prune dead sessions
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Ensure caffeinate process is started by the daemon (not individual sessions) and tracked in daemon.state.json to prevent it from becoming a runaway process
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Emit 'machine-update-state' WebSocket events whenever daemon status changes, including expectedVersion for optimistic locking and handle version-mismatch responses by syncing to current server version
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : During daemon startup, perform version check by reading daemon.state.json and comparing startedWithCliVersion with configuration.currentCliVersion; if version mismatch detected, call stopDaemon() to kill old daemon before proceeding
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Write daemon state including PID, HTTP port, version, and start time to daemon.state.json upon successful startup; maintain 'state' and 'stateReason' fields to explain daemon status and preserve file on exit/stop instead of hard deleting
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/doctor/**/*.{ts,tsx} : Implement 'happy doctor clean' command that calls findRunawayHappyProcesses() to filter likely orphans, then killRunawayHappyProcesses() to send SIGTERM, wait 1 second, and send SIGKILL if still alive
Applied to files:
src/daemon/run.tssrc/ui/doctor.tssrc/utils/tmux.real.integration.test.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Perform authentication via authAndSetupMachineIfNeeded() during daemon startup to ensure credentials exist before establishing backend connections
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/src/index.ts : In CLI startup phase, check if machine ID exists in settings; if not, create ID locally only without creating machine on server (daemon's responsibility on startup)
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : On daemon startup, POST to /v1/machines REST endpoint with encrypted metadata and daemonState, sending machineId, and receive metadataVersion and daemonStateVersion from server response
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement daemon state structure containing status ('running' | 'shutting-down' | 'offline'), pid, httpPort, startedAt, shutdownRequestedAt, and shutdownSource as dynamic daemon runtime state
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement stopSession() to find session by happySessionId, send SIGTERM to process, and remove from tracking map on exit event
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/api/**/*.{ts,tsx} : Implement session isolation through unique session IDs
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement notifyDaemonSessionStarted() webhook endpoint that receives POST requests to /session-started and creates TrackedSession entries with startedBy identifier
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement HTTP endpoint /spawn-session that creates new sessions (used by integration tests) via daemon HTTP server
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Register RPC handlers for 'spawn-happy-session', 'stop-session', and 'requestShutdown' on the daemon's HTTP server and WebSocket connection
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Use acquireDaemonLock() to create an exclusive lock file with O_EXCL flag to prevent multiple daemon instances from running simultaneously
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to src/{auth,persistence}.{ts,tsx} : Store private keys in `~/.handy/access.key` with restricted permissions
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : Implement cleanupAndShutdown() to clear heartbeat interval, update daemon state to 'shutting-down' on backend, disconnect WebSocket, stop HTTP server, release lock file, and write final daemon state before exit
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : In stopDaemon() from controlClient.ts, read daemon.state.json and attempt graceful shutdown via HTTP POST to /stop endpoint, falling back to process.kill(pid, 'SIGKILL') if HTTP fails
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:38:16.567Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: src/daemon/CLAUDE.md:0-0
Timestamp: 2026-01-12T11:38:16.567Z
Learning: Applies to src/daemon/**/daemon/**/*.{ts,tsx} : On daemon shutdown, resolve the shutdown promise when OS signals (SIGINT/SIGTERM) are received, HTTP /stop endpoint is called, RPC requestShutdown is invoked, or uncaught exceptions occur
Applied to files:
src/daemon/run.ts
📚 Learning: 2026-01-12T11:37:46.296Z
Learnt from: CR
Repo: leeroybrun/happy-cli PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T11:37:46.296Z
Learning: Applies to **/*.test.{ts,tsx} : Colocate test files with source files using `.test.ts` naming convention
Applied to files:
src/utils/tmux.real.integration.test.ts
🧬 Code graph analysis (2)
src/daemon/run.ts (3)
src/utils/spawnHappyCLI.ts (1)
buildHappyCliSubprocessInvocation(65-87)src/utils/tmux.ts (1)
TmuxUtilities(374-956)src/daemon/types.ts (1)
TrackedSession(11-22)
src/ui/doctor.ts (1)
src/persistence.ts (1)
readSettings(263-312)
🔇 Additional comments (27)
src/ui/doctor.test.ts (1)
1-10: LGTM! Test correctly validates secret redaction for env templates.The test appropriately verifies the key security behaviors:
- Bare
${VAR}templates are displayed as-is (no embedded secrets)- Templates with
:-or:=default values have their fallbacks maskedThe dynamic import pattern works well for testing the module's exports.
src/ui/doctor.ts (3)
20-39: Well-designed redaction with proper handling of shell-style env templates.The implementation correctly:
- Preserves safe
${VAR}templates that don't embed secrets- Masks fallback values in
${VAR:-secret}and${VAR:=secret}patterns- Uses recursion to handle fallbacks consistently
One consideration: the regex patterns only match uppercase variable names (
[A-Z_][A-Z0-9_]*). Lowercase env vars like${my_key:-secret}would be fully masked as<N chars>rather than preserving the template structure. This is still secure, just less informative in doctor output.
41-68: LGTM! Clean redaction of sensitive profile data.The function properly:
- Deep clones to avoid mutating the original settings
- Removes legacy
localEnvironmentVariablesthat may contain secrets- Applies
maskValueto all environment variable values in profilesThe defensive
Array.isArraychecks andhasOwnPropertyusage are appropriate.
170-177: LGTM! Settings are properly redacted before display.The redaction is correctly applied when outputting settings to console, ensuring secrets with default values aren't leaked in doctor diagnostics.
src/daemon/run.ts (21)
36-45: LGTM!The implementation correctly filters out undefined values from
NodeJS.ProcessEnvand merges withextraEnvgiving precedence to the extra environment variables.
47-82: LGTM!The function correctly assembles tmux spawn configuration with proper environment handling. The extraction of
TMUX_TMPDIRintotmuxCommandEnvensures the tmux client uses the correct socket directory, addressing the PR objective of passingTMUX_TMPDIRto tmux client invocations.
186-186: LGTM!The
codexHomeDirCleanupByPidmap correctly tracks cleanup callbacks keyed by PID, following the existing pattern used forpidToTrackedSession. Based on learnings, tracking spawned processes and their resources for cleanup is the expected pattern.
237-252: Good security practice: sanitized logging.The logging now properly redacts sensitive information by logging only metadata (keys, counts, boolean flags) rather than actual values. This aligns with the PR objective of secret redaction in logs.
257-258: LGTM!The
codexHomeDirCleanupArmedflag is a good defensive pattern to prevent double-cleanup scenarios where the temp directory might be cleaned up both by error handling and by the PID-based cleanup system.
330-342: LGTM!The logic correctly implements the PR objective of only applying profile environment variables when explicitly provided by the caller (GUI), without falling back to CLI-local active profile. This is a security-conscious design that respects GUI opt-in gating.
344-352: LGTM!The implementation correctly threads
profileIdthrough to the child environment viaHAPPY_SESSION_PROFILE_ID. The explicit!== undefinedcheck is correct to allow empty strings (meaning "no profile"), and the merge order ensures auth variables cannot be overridden by profile settings.
382-385: LGTM!Correctly cleans up the temporary CODEX_HOME directory when authentication variable expansion fails, but only if the cleanup hasn't been armed (registered with the PID-based cleanup system).
416-423: LGTM!The integration correctly uses
buildTmuxSpawnConfigand passestmuxCommandEnvtoTmuxUtilities, ensuringTMUX_TMPDIRflows to tmux client invocations as specified in the PR objectives.
426-430: LGTM!The
spawnInTmuxcall correctly passes the merged environment (tmuxEnv) ensuring tmux and non-tmux spawns behave consistently, as stated in the PR objectives.
440-441: LGTM!Correctly resolves the actual tmux session name used (important when the input session name was empty, indicating "use current/most recent session"). The fallback chain is reasonable for defensive messaging.
449-451: LGTM!The improved messages now reference the resolved
tmuxSessionvariable, providing users with accurate attach commands regardless of how the session was specified.
456-459: LGTM!Correctly registers the CODEX_HOME cleanup callback with the PID-based tracking system for tmux-spawned processes.
543-546: LGTM!Correctly cleans up the CODEX_HOME temporary directory when process spawning fails (no PID returned), preventing resource leaks.
564-567: LGTM!Consistent with the tmux spawning path, correctly registers cleanup callback for regular process spawns.
617-620: LGTM!Proper cleanup in the catch block ensures CODEX_HOME temporary directories are cleaned up even when unexpected errors occur during session spawning.
669-677: LGTM!The cleanup logic correctly handles child process exit events: removes from tracking map first (preventing double cleanup), then calls the cleanup callback with proper error handling.
758-767: LGTM!The heartbeat loop correctly handles cleanup for stale sessions, particularly important for tmux-spawned processes where exit events may not be received since they're not direct child processes of the daemon.
771-784: LGTM!This additional cleanup pass handles an edge case where
stopSessionmight remove a session from tracking while the cleanup callback remains. The logic correctly verifies the process is dead before cleaning up, preventing premature removal of temp directories still in use.
520-520: LGTM!Good documentation clarifying that
sessionIdis not currently used for session resumption.
316-326: The temp directory implementation is secure. Thetmplibrary v0.2.5 (specified in package.json) creates directories with default mode0o700(owner rwx only) and files with mode0o600(owner rw only), which are appropriate for sensitive token storage. The code also includes comprehensive cleanup management throughcodexHomeDirCleanupByPidmap and proper error handling with logging at each cleanup point. No changes are required.src/utils/tmux.real.integration.test.ts (2)
17-89: Helper utilities are clear and well-isolated.Nice separation of tmux gating, polling, dump script creation, and env cleanup keeps the integration suite predictable.
98-119: runTmux isolation logic looks good.Centralizing the env scrub keeps these tests from leaking user tmux context.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const listedPid = Number.parseInt(panes.stdout.trim(), 10); | ||
| expect(listedPid).toBe(result.pid); | ||
|
|
||
| await waitForFile(outFile, 2_000); |
There was a problem hiding this comment.
Consider a shared, slightly larger file-wait timeout to reduce flakiness.
2 seconds can be tight on slow CI or cold tmux startup. A shared constant (e.g., 5s, or derived from the suite timeout) improves stability.
✅ Example adjustment
+const FILE_WAIT_TIMEOUT_MS = 5_000;- await waitForFile(outFile, 2_000);
+ await waitForFile(outFile, FILE_WAIT_TIMEOUT_MS);- await waitForFile(outFile, 2_000);
+ await waitForFile(outFile, FILE_WAIT_TIMEOUT_MS);- await waitForFile(outFile, 2_000);
+ await waitForFile(outFile, FILE_WAIT_TIMEOUT_MS);- await waitForFile(outFile, 2_000);
+ await waitForFile(outFile, FILE_WAIT_TIMEOUT_MS);Also applies to: 192-192, 228-228, 276-276
🤖 Prompt for AI Agents
In `@src/utils/tmux.real.integration.test.ts` at line 150, The test uses a tight
2_000 ms hardcoded timeout in waitForFile calls which causes flakiness; create a
shared constant (e.g., const FILE_WAIT_TIMEOUT_MS = 5_000 or derive from the
suite timeout) and replace the literal 2_000 in waitForFile(outFile, 2_000) at
the occurrences inside src/utils/tmux.real.integration.test.ts (including lines
where waitForFile is called around the reported locations) with
waitForFile(outFile, FILE_WAIT_TIMEOUT_MS), and update any other waitForFile
invocations (e.g., the ones noted at the other call sites) to use the same
constant so the timeout is consistent and easier to tweak for CI.
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
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.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
Improvements
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.